Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fixing jshint single quotes issues #904

Merged
merged 2 commits into from
Sep 28, 2015

Conversation

jloveland
Copy link
Contributor

fixing jshint issues by requiring single quotes

@@ -12,6 +12,7 @@
"undef": true, // Require all non-global variables be declared before they are used.
"unused": false, // Warn unused variables.
"strict": true, // Require `use strict` pragma in every file.
"quotmark": "single", // Require only single quotes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lirantal and @ilanbiala , can we keep quotmark in here until #763 is addressed? Reason being is that I missed a lot of the double quote issues in my last PR.

Copy link
Member

Choose a reason for hiding this comment

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

@jloveland I don't think it's really that necessary to just make a temporary commit..I'd rather wait a few days for ESLint.

@lirantal
Copy link
Member

Thanks @jloveland
Were you able to actually reproduce the error where this happens? I remember reading in the other PR thread that it didn't happen to you...

@lirantal lirantal added this to the 0.4.2 milestone Sep 11, 2015
@lirantal lirantal self-assigned this Sep 11, 2015
@jloveland
Copy link
Contributor Author

@lirantal, without "quotmark": "single", in the .jshintrc file, it doesn't check for single quotes when I run grunt jshint. Will ESLint fix this?

@jloveland jloveland changed the title fixing jshint issues by requiring single quotes fixing jshint single quotes issues Sep 11, 2015
@lirantal
Copy link
Member

Yes, whenever @rhutchison will move forward with that ESLint PR...

@lirantal
Copy link
Member

Since we are embracing the single quotes all around the project then this PR LGTM, and once we have linting for code styles then this will be enforced too.

@ilanbiala do you have any other thoughts about this?

{ color: "info", progress: "60"},
{ color: "primary", progress: "80"},
{ color: "success", progress: "100"}
{ color: 'danger', progress: '20' },
Copy link
Member

Choose a reason for hiding this comment

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

consistent spacing in this array of objects please

@ilanbiala
Copy link
Member

Nope, LGTM.

@jloveland
Copy link
Contributor Author

@ilanbiala I fixed spacing
@lirantal, without "quotmark": "single", in the .jshintrc file, it doesn't check for single quotes when I run grunt jshint. I can add it back in or we can wait for @rhutchison ESlint.
Let me know what you prefer and I can squash if it's ready to merge.

@ilanbiala
Copy link
Member

@jloveland we are going to add ESLint, so no need to add it back.

@lirantal
Copy link
Member

@ilanbiala looks like we're delaying quite a bit with the ESLint PR so I recommend if we don't get that PR in the next few days from @rhutchison then let's just create it ourself. It is much more important now that every PR needs to conform to some kind of style.

@codydaig
Copy link
Member

@lirantal I know @rhutchison has started on it. But hasn't been touched in a little bit.

https://github.com/gym/mean/tree/eslint

@lirantal
Copy link
Member

I remember we already have some issue which discusses code formatting but if we don't lets open one and put everything there for a discussion.

I'll merge this one in.

lirantal added a commit that referenced this pull request Sep 28, 2015
@lirantal lirantal merged commit cc80930 into meanjs:master Sep 28, 2015
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.

4 participants