search

jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.3k stars 5.53k forks source link

2850 #2904

Closed mohammadali-seifkashani closed 3 years ago

jgonggrijp commented 3 years ago

@mohammadali-seifkashani Thank you for your continued efforts.

I can tell that you meant to follow up on my comments in #2897, but you applied them to your findKeys implementation instead of your transition implementation. I asked you in #2898 to not mix these features. Since #2681 already has a good implementation of findKeys, I would strongly recommend that you focus all your effort on transition.

You can fix this as follows:

  1. git checkout 2850.
  2. git reset --hard fc3d6a0: this will remove the commits related to findKeys from your 2850 branch. Don't worry, the findKeys commits are still on your 2676 branch.
  3. At this point, make sure that findKeys is really gone. If it's still there, delete it manually.
  4. npm run install: this will install commit hooks that ensure that the underscore.js and the underscore-esm.js bundles stay in sync with the source code. I can tell from the current PR that you haven't done this yet.
  5. Process my comments from https://github.com/jashkenas/underscore/pull/2897#issuecomment-748299380 again, but now applying them to transition. Commit your changes.
  6. npm run test: if any test fails, fix your code and commit again.
  7. git push --force.

At this point, the current PR (#2904) should only contain your transition-related contributions. After this, I'll do a detailed code review over here. You don't need to open another PR. Thanks in advance!

mohammadali-seifkashani commented 3 years ago

OH I made a mistake. More than mistake. In this PR and #2898 mistakenly I pulled request for irrelevant issue. I'm sorry :(

jgonggrijp commented 3 years ago

@mohammadali-seifkashani It's alright. Please do feel welcome to continue #2897.