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

add customizable sort order, skip and limit options on match list #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

djulien
Copy link

@djulien djulien commented Sep 17, 2015

Added a few features:

  • allow custom sort order to be specified (anything that JavaScript sort accepts)
  • allow entries at start of match list to be skipped
  • allow match list to be truncated

Added a few features:
- allow custom sort order to be specified (anything that JavaScript sort
accepts)
- allow entries at start of match list to be skipped
- allow match list to be truncated
@call-a3
Copy link
Collaborator

call-a3 commented Sep 20, 2015

Awesome, thanks for adding this functionality!

Some remarks on the way you submitted this pull request though...

  • We use git-flow during development, so we only accept pull requests on the develop branch.
  • We use SemVer for versioning require-globify, and since you added new functionality rather than a bugfix, the new version number would have to be 1.4.0 rather than 1.3.1.
  • I try to keep require-globify fully tested, so I'd really appreciate it if you added some specific tests for this new functionality to ensure future development doesn't break these features.

Sorry to have to nitpick about this, but I really believe it's for best in the future...

@djulien
Copy link
Author

djulien commented Oct 4, 2015

Sorry to have to nitpick about this, but I really believe it's for best in the future..

Yes, I agree. I only know a few very basic git commands and there is so much info and styles out there that I find it overwhelming, so any tips are helpful.

We use SemVer for versioning require-globify, and since you added new functionality rather than a
bugfix, the new version number would have to be 1.4.0 rather than 1.3.1.

I wasn't sure about that (since the changes were backwards compatible I used the smaller increment). thanks for clarifying.

I try to keep require-globify fully tested, so I'd really appreciate it if you added some specific tests for
this new functionality to ensure future development doesn't break these features.

I can add some tests for these changes if it would be helpful. I didn't see any tests in there initially, but now that I look at it again I guess the "specs" are the tests. Sorry about that. How detailed should they be? (is 1 test per feature enough, or you want edge cases and negative tests covered as well?)

We use git-flow during development, so we only accept pull requests on the develop branch.

I wasn't aware of the methodology you used. I have now read thru that link but I am still not quite sure exactly how I should do that. If you could list the sequence of commands that I should use (for the tests or future changes) I'll follow it, otherwise I'll just leave things are they are now.

@call-a3
Copy link
Collaborator

call-a3 commented Oct 6, 2015

The confusion is entirely my fault, since I didn't put up any instructions for contributors anywhere. I have fixed that by now, by putting up info in the README (on the develop branch for now).

With regard to tests, I believe some basic tests (or specs) covering every feature/aspect is enough. If you already can think of edge cases which are likely to occur, it might also be good to add a test for them, but I don't think you should go out of your way to find each and every possible one. If these kinds of edge cases crop up, they'll be reported as bugs and as such tests will be added for that specific case if need be. A similar mindset may be applied to negative tests.

Git flow is more of a methodology than anything else. There's a nice article linked from the README on the develop branch that explains it. In this case you'd basically do the following:

  • make a fork of the repo, (you already did this)
  • start a new branch off of develop for your new feature git checkout -b feature-match-options develop
  • when you're done working on the feature, create a pull request to merge back with develop

Thanks for your understanding! I hope we can merge and release this helpful new feature soon!

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