search

mariocasciaro / scatter

IoC container and out-of-the-box extensibility for Node.js applications
MIT License
154 stars 14 forks source link

fix #15 #16

Closed k7sleeper closed 10 years ago

k7sleeper commented 10 years ago

make npm test beeing able to be executed under Windows

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 8c326cbcbbaba073776986a0e35d6f8651ee07f6 on k7sleeper:master into 05299fad339e12390481a4d9baac87b3136c25d1 on mariocasciaro:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 149a31f9826ba039d48f5398db56879082b3c737 on k7sleeper:master into 05299fad339e12390481a4d9baac87b3136c25d1 on mariocasciaro:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 149a31f9826ba039d48f5398db56879082b3c737 on k7sleeper:master into 05299fad339e12390481a4d9baac87b3136c25d1 on mariocasciaro:master.

mariocasciaro commented 10 years ago

I think your editor is autoformatting the code, the formatting is different from the rest of the project (indentation, etc.). It is important we keep the same conventions throughout the codebase and beside that it makes difficult to review the diff. Can you please change the formatting and amend the commit?

k7sleeper commented 10 years ago

Yes, of course, I'll send you a corrected PR.

k7sleeper commented 10 years ago

I've just changed the formatting. Is it ok?

mariocasciaro commented 10 years ago

The formatting seems better, but I prefer if you amend commit (149a31f) instead of creating a new one, as it is now it's still hard from the diff to see what you effectively changed in the code ( https://github.com/k7sleeper/scatter/commit/6f283450f8770fbcdfc947a60369f07d9f58f3a6 )

k7sleeper commented 10 years ago

Where/how can I delete that commit?

mariocasciaro commented 10 years ago

To delete a commit http://stackoverflow.com/questions/1338728/how-can-i-delete-a-git-commit

k7sleeper commented 10 years ago

Does it make sence to store an .editorconfig at the project root, doesn't it?

mariocasciaro commented 10 years ago

Please do not commit any editor configuration, they are specific to each developer and should not be shared in the repo.

k7sleeper commented 10 years ago

Well, EditorConfig helps developers define and maintain consistent coding styles between different editors and IDEs. There're also projects out there which have a .editorconfig checked-in in the repo. I don't know whether it really helps keeping the code consistent. Anyway, the existence of that file could be an important reference to your coding style, but as you already stated, each commit to a repo should follow the existing conventions, even without a .editorconfig. I guess a .editorconfig could help learning your coding conventions.

mariocasciaro commented 10 years ago

I'm a little bit skeptical about EditorConfig itself, because it is ultimately up to the developer to follow some simple rules. But just to clarify, I'm generally not so fussy about formatting, but on the other side it's really important that the formatting of existing code itself it's not modified between commits, since beside violating existing style it adds a lot of noise into code diffs.

That said, I don't want to totally shut down your proposal of using the .editorconfig if this can help you or other contributors following the current formatting styles. Let's give it a try, but I need to ask you a favor, can we start from scratch with this pull request please? There are 9 commits attached to this PR, I suggest you to create a new PR with only 1 commit for the test script issue + another PR with only 1 commit for the directory require feature (and include the editorconfig also if you want). This way it will be easier for ourselves and other users to understand what's going on between commits.

My suggestion: save your current code somewhere, sync your fork with the HEAD of the main Scatter repo, reapply your changes (with proper formatting and the other suggestions I gave you in the code review for 3333ecf ) and resubmit the 2 commits in 2 separate PRs. I know all this is a pain but it is normal when a new contributor approaches a project, after we clarify and share some initial thoughts, the following contribution will be definitely smoother, and we are almost there.

After you create the new PRs I'll close this one ok?

k7sleeper commented 10 years ago

I fully agree!

k7sleeper commented 10 years ago

Because I'm a Git greenhorn, I just deleted my fork and first changed package.json from GitHub to solve this issue. That task created a fork again and made a PR (#18).

I think you can accept the new PR and than close #15 and #18.

After that I send a PR for #12.

Is that ok?