Skip to content
This repository has been archived by the owner on Dec 29, 2020. It is now read-only.

Minor improvements #33

Merged
merged 11 commits into from
Feb 24, 2014
Merged

Minor improvements #33

merged 11 commits into from
Feb 24, 2014

Conversation

XhmikosR
Copy link
Contributor

Let me know if you want me to make any changes and I'll rebase the branch.

@nschonni
Copy link
Contributor

Anytime you do an npm install or update, NPM will reformat to 2 spaces

@markelog
Copy link
Member

Anytime you do an npm install or update, NPM will reformat to 2 spaces

What?

@gustavohenke
Copy link
Member

@nschonni no, it'll not, unless you pass --save.


"node": true
"undef": true,
"unused": true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of sorting this kind of stuff, creates additional complicity to something very simple.
Indentation is also debatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always follow alphabetical order because it simply is easier to spot the option I'm looking for without looking randomly or using search. As for indentation, I don't really care either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctrl+f/cmd+f? If someone would want to add something, he would have to sort this list to figure it out, which sounds...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. I simply do Alt+O in the same editor I use and the values are sorted by name.

@markelog
Copy link
Member

2 spaces is the default for package.json

It's better to be consistent i think, if we use four spaces everywhere else it's weird to do it differently in one other file.

@XhmikosR
Copy link
Contributor Author

I disagree. package.json should follow what npm init does. But either way, I don't really mind.

I mostly care about the rest of the changes.

@markelog
Copy link
Member

I don't really mind.

Great, other changes looks good to me

@nschonni
Copy link
Contributor

@markelog sorry I was a little unclear in the previous comment, but @gustavohenke clarified that anytime NPM modifies the package.json, it will change the indenting of the file to 2 spaces.

@XhmikosR
Copy link
Contributor Author

So do I rebase to revert the package.json and jshintrc indentation changes? I'm still in favor of those changes for the reasons I explained above, but the final decision is for the project maintainers to make.

@markelog
Copy link
Member

So do I rebase to revert the package.json and jshintrc indentation changes?

I would prefered, yes, but final decision on @gustavohenke

@gustavohenke
Copy link
Member

About .jshintrc: whatever, keep it as is, it's less work :)
About package.json: I prefer to keep the indentation consistent with other files, so, please, revert this one.

Squash and when you're done and we're good to merge.

@XhmikosR
Copy link
Contributor Author

Branch rebased. I kept the jshintrc changes.

Squash doesn't make sense here; each patch is for a different thing.

@gustavohenke gustavohenke merged commit 92e18c2 into jscs-dev:master Feb 24, 2014
@XhmikosR
Copy link
Contributor Author

Thanks!

BTW, maybe you should add a License for the project?

@gustavohenke
Copy link
Member

Why not!

@XhmikosR
Copy link
Contributor Author

@gustavohenke: feel free to add it, I don't know which license you prefer :)

@XhmikosR
Copy link
Contributor Author

BTW, any plans to make a new npm package after you add the license? The changes aren't big but can be useful.

@gustavohenke
Copy link
Member

About the license, I'm waiting for remy/mit-license#452. Too lazy to customize my own :P
The new version... well, we can do it, I was waiting for a new version of jscs, but its taking too long.

@XhmikosR
Copy link
Contributor Author

I added the license in #34

@XhmikosR
Copy link
Contributor Author

Maybe wait a couple more days; there will be a new jscs release soon-ish hopefully.

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

Successfully merging this pull request may close these issues.

None yet

4 participants