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

standardize ' vs " [#151046132] #45

Closed
nicolasferraro opened this issue Oct 5, 2017 · 8 comments · Fixed by #59
Closed

standardize ' vs " [#151046132] #45

nicolasferraro opened this issue Oct 5, 2017 · 8 comments · Fixed by #59

Comments

@nicolasferraro
Copy link
Collaborator

nicolasferraro commented Oct 5, 2017

Currently ' and " are used interchangeably in the code, for the sake of standardization and code legibility we should pick one as the standard and fix the remaining quotes on the code.

@cannawen Do you have any preference between ' and "?

Maybe a coding guideline of sorts could be defined so everyone is on the same page. (tabs vs spaces, space between brackets, etc)

@cannawen
Copy link
Owner

cannawen commented Oct 5, 2017

Yes, coding guidelines would be an excellent idea! I personally think ' looks better, but maybe it's worth researching what JavaScript standards are? I'm not too picky on what the formatting is, as long as it's consistent.

Maybe we can find a linter that automatically runs and enforces style, to reduce the human effort required? I haven't looked into it much so I don't know if such a thing exists, but I'm guessing it should

@nicolasferraro
Copy link
Collaborator Author

I think adding jshint to travis-ci should be quite easy. This way we could configure jshint through .jshintrc and have it run on every build.

@cannawen
Copy link
Owner

cannawen commented Oct 5, 2017

That looks like an interesting tool! Do you want to take a stab at setting it up @nicoferraro96 ?

@nicolasferraro
Copy link
Collaborator Author

Yeah sure!

@chazzlabs
Copy link
Collaborator

I think this makes a lot of sense, especially now that more people are contributing as a result of Hacktoberfest.

@nicoferraro96 Are you thinking of extending an existing style guideline with jshint or are you going to add a few rules just as a sort of starting point?

@nicolasferraro
Copy link
Collaborator Author

@chazzlabs My idea was to base the config on an existing style guideline and maybe add/remove some rules, depending on if they make sense or not for this project. If you have any suggestions I would be glad to hear them

@chazzlabs
Copy link
Collaborator

@nicoferraro96 I don't know of any popular style guides for jshint. Personally I use eslint and usually use AirBnb's style guide, but that's really more for front-end development.

I like using some other popular guide as a base because it feels like I'm starting with something that's more "standard" within the community. I'd only be concerned about the style guide potentially requiring a lot of changes to fix the initial batch of issues it might bring up. In that case, overriding some rules like you mentioned will probably help a lot. I just wouldn't want anyone who's contributing to be overwhelmed or confused by their code suddenly not meeting the guidelines.

@nicolasferraro
Copy link
Collaborator Author

nicolasferraro commented Oct 5, 2017

@chazzlabs I had forgotten about airbnb's eslint config, now that you mention it I think I'm going to try that first.

Maybe what we could do is first adding the linter and the corresponding travis config, but not enable it and update the contribute.md encouraging contributors to use the linter and comply with it as much as possible.

We could also open a separate issue to bring the current code up to standard.

cannawen added a commit that referenced this issue Oct 6, 2017
Add eslint with airbnb base style guide [#45]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants