Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed 2 issue #162

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fixed 2 issue #162

wants to merge 8 commits into from

Conversation

mehdi89
Copy link

@mehdi89 mehdi89 commented Jun 10, 2018

No description provided.

@mehdi89 mehdi89 closed this Jun 10, 2018
@mehdi89 mehdi89 reopened this Jun 10, 2018
@coffeeburrito
Copy link
Contributor

I have some suggestions for your pull request:

  • Don't change the package name
  • Append/update your changes to readme.md instead of removing everything.
  • Remove unnecessary blank/commented lines (stuff like "// dd($params);")
  • Consider that ES6.0 is already supported in the master branch. If you think something was missed, please elaborate on what problem your code fixes.
  • Due to order of operations, your searchByQuery change overwrites sourceFields if set, but then itself gets overwritten by aggregations and sort if set. This is inconsistent and undesirable behaviour. Why not write a new searchByJSON() method instead?

@mehdi89
Copy link
Author

mehdi89 commented Jun 14, 2018

Thank for the suggestions and time to review. submitting this pull request was a mistake. I was doing this fix really quickly for my project and mistakenly submitted for pull request. Can you please remove this? and If i got chance will try to create the function as you suggested searchByJSON()

Thank you.

@coffeeburrito
Copy link
Contributor

Sorry, I'm just some random dude and not a maintainer for this project. Also stack overflow suggests that only GitHub staff can delete pull requests. Repo owners are limited to closing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants