os-js / osjs-client

OS.js Client Module
https://manual.os-js.org/
Other
30 stars 30 forks source link

add feature search engine adapater #202

Closed mahdi00021 closed 1 year ago

mahdi00021 commented 1 year ago

please check this "add feature search engine adapater"

mahdi00021 commented 1 year ago

Removing package-lock.json doesn't have an impact on the purpose of this PR, please restore the file so we can keep the changes focused on a single thing :)

@ajmeese7

I didn't touch the package-lock.json, I just checked and corrected the errors with eslint

The file is available

andersevenrud commented 1 year ago

I didn't touch the package-lock.json

You did. You can see this in this PR if you go to the changes tab.

andersevenrud commented 1 year ago

You also changed some test files. This should be reverted because it's out of scipe of this PR.

mahdi00021 commented 1 year ago

I didn't touch the package-lock.json

You did. You can see this in this PR if you go to the changes tab.

Automatic eslint has changed

andersevenrud commented 1 year ago

Eslint does not touch package lock. It also doesn't change the test files unless you tell it to do so.

So you must have done this by accident or something....

mahdi00021 commented 1 year ago

Eslint does not touch package lock. It also doesn't change the test files unless you tell it to do so.

So you must have done this by accident or something....

I commit now please check again

mahdi00021 commented 1 year ago

Now I made the changes and corrections, there is nothing

andersevenrud commented 1 year ago

Squash your commits and write a commit message that follows the conventional commits spesification, i.e.: feat(search): adapter support.

mahdi00021 commented 1 year ago

What should I do now? delete branch again? :)

andersevenrud commented 1 year ago

You can use git interactive rebase to squash + change the commit message and do a force push.

No need to create new PR or branches. Git has the tools you need.

mahdi00021 commented 1 year ago

You can use git interactive rebase to squash + change the commit message and do a force push.

No need to create new PR or branches. Git has the tools you need.

please check this commit

andersevenrud commented 1 year ago

You need to do a squash. This PR should only contain 1 commit.

mahdi00021 commented 1 year ago

You need to do a squash. This PR should only contain 1 commit.

@andersevenrud please check

mahdi00021 commented 1 year ago

I tested it in my home system and it was executed successfully

andersevenrud commented 1 year ago

I tested it in my home system and it was executed successfully

But it needs to be verified by another human first since this stuff is not covered in automated tests.

mahdi00021 commented 1 year ago

I tested it in my home system and it was executed successfully

But it needs to be verified by another human first since this stuff is not covered in automated tests.

Can you do MERGE now?

andersevenrud commented 1 year ago

No. Changes must be verified before it can be merged. I just explained why.

mahdi00021 commented 1 year ago

No. Changes must be verified before it can be merged. I just explained why.

Do you have to do this?

andersevenrud commented 1 year ago

Yes. No code gets merged before it has been verified either by another human or automated tests.

This is a standard process in programming.

mahdi00021 commented 1 year ago

Yes. No code gets merged before it has been verified either by another human or automated tests.

This is a standard process in programming.

So I will wait for you to accept merge

andersevenrud commented 1 year ago

Can you do a git rebase onto the master branch?

I've added a unit test for the search service, and you have to run a rebase in order for this PR to run that test.

andersevenrud commented 1 year ago

It would also be nice if you added a test case for adding custom adapters and ensure that they return results.

mahdi00021 commented 1 year ago

It would also be nice if you added a test case for adding custom adapters and ensure that they return results.

I added custom adapters for ensure that they return results in my system now what i do?

andersevenrud commented 1 year ago

now what i do?

First do a git rebase. On your branch:

git fetch origin master:master
git rebase master
git push --force
mahdi00021 commented 1 year ago

now what i do?

First do a git rebase. On your branch:

git fetch origin master:master
git rebase master
git push --force

I do rebase , git said : Everything up-to-date

andersevenrud commented 1 year ago

Everything up-to-date

Oh... yes. You have a fork. So you need to add an upstream and use that instead of origin.

https://www.atlassian.com/git/tutorials/git-forks-and-upstreams

mahdi00021 commented 1 year ago

i add upstream your url or my fork url?

mahdi00021 commented 1 year ago

Everything up-to-date

Oh... yes. You have a fork. So you need to add an upstream and use that instead of origin.

https://www.atlassian.com/git/tutorials/git-forks-and-upstreams

i do please check

andersevenrud commented 1 year ago

FYI: Github sends out notifications whenever a PR is updated. You don't have to comment after you push changes.


You also should have gotten a notification yourself now that the tests have run, and failed.

So the next step now is to update the tests so that they pass.

Are you familiar with unit testing ?

mahdi00021 commented 1 year ago

FYI: Github sends out notifications whenever a PR is updated. You don't have to comment after you push changes.

You also should have gotten a notification yourself now that the tests have run, and failed.

So the next step now is to update the tests so that they pass.

Are you familiar with unit testing ?

I'm confused, I changed everything you said, I put it on, it was successful, now you said do rebase, I did it, the test failed. why?

Previously, the tests were successful

andersevenrud commented 1 year ago

Previously, the tests were successful

I added a unit test for the search so this could be automatically checked that it's working as intended.

You did a rebase, so now this test is running in this PR.

Since you changed the API of the search service, this test will fail. It needs to be updated to use the changes you introduced, which is the new options argument in the Search class.

If you look at the failed test that you got a notification for it says exactly this.

mahdi00021 commented 1 year ago

Previously, the tests were successful

I added a unit test for the search so this could be automatically checked that it's working as intended.

You did a rebase, so now this test is running in this PR.

Since you changed the API of the search service, this test will fail. It needs to be updated to use the changes you introduced, which is the new options argument in the Search class.

If you look at the failed test that you got a notification for it says exactly this.

Previously, the tests were successful

I added a unit test for the search so this could be automatically checked that it's working as intended.

You did a rebase, so now this test is running in this PR.

Since you changed the API of the search service, this test will fail. It needs to be updated to use the changes you introduced, which is the new options argument in the Search class.

If you look at the failed test that you got a notification for it says exactly this.

Excuse me, how should I write a test? I am not familiar with the test writing of your project. In fact, my main expertise is backend PHP python. I got to know Node a few weeks ago

andersevenrud commented 1 year ago

Excuse me, how should I write a test? I am not familiar with the test writing of your project. In fact, my main expertise is backend PHP python. I got to know Node a few weeks ago

You'd write a test just the same way as you would do for PHP or Python. The only difference here is the syntax, really.

It's only a matter of:

https://github.com/os-js/osjs-client/blob/d6781762ce7948a5a9c19e16d1917f715b38de82/__tests__/search.js#L39

andersevenrud commented 1 year ago

add-feature-search-engine-adapater-by-mahdi00021-·-Pull-Request-202-·-os-js-osjs-client

Not sure what you did, but now it's even more changes in here than before :sweat_smile:

Are you working in an outdated repository ? If you create a an upstream (like discussed earlier) you can keep it up-to-date and then rebase your branch onto master branch to make sure it's in sync.

mahdi00021 commented 1 year ago

add-feature-search-engine-adapater-by-mahdi00021-·-Pull-Request-202-·-os-js-osjs-client

Not sure what you did, but now it's even more changes in here than before sweat_smile

Are you working in an outdated repository ? If you create a an upstream (like discussed earlier) you can keep it up-to-date and then rebase your branch onto master branch to make sure it's in sync.

I apply : git reset --soft HEAD~4 && git commit --no-verify -m "feat(search): add feature search adapter support" git push --force origin featureSaerchEngine

for become to 1 commit and then be fails :((

Added and changed several auto files

andersevenrud commented 1 year ago

A git reset <ref> will reset your changes.

If you have multiple commits you want to join then use interactive rebase.

For example, if you have 10 commits: git rebase -i HEAD~10. Now, on the bottom 9 change pick to squash. This will squash the selected 9 onto the above making one 1 in total (10 - 9). Then this can be force pushed.