capaj / require-globify

transform for browserify, which allows to require files with globbing expressions
MIT License
70 stars 11 forks source link

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

Open djulien opened 8 years ago

djulien commented 8 years ago

Added a few features:

call-a3 commented 8 years ago

Awesome, thanks for adding this functionality!

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

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

djulien commented 8 years ago

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 commented 8 years ago

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:

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