Skip to content

add editorconfig & jshint. - #92

Merged
rakyll merged 2 commits into
googleapis:masterfrom
stephenplusplus:tools
Aug 5, 2014
Merged

add editorconfig & jshint.#92
rakyll merged 2 commits into
googleapis:masterfrom
stephenplusplus:tools

Conversation

@stephenplusplus

Copy link
Copy Markdown
Contributor

I know these crazy change sets aren't fun. Sorry :(

This PR introduces editorconfig and JSHint. The configuration files are at root level, then I went through and ran it against the code, which is how we got to the huge diff. There are some things I'm not crazy about; mainly the 80 character limit. There are a few cases in here where splitting into a new line just makes things ugly.

I hate to even say this, being that the diff is so huge, but this is still just an initial "sweep." There are other code-style things that I think can still be addressed.

Please take a look. You'll notice lots of new lines, semi-colons, variable renaming, variable declaring, use strict atop every file, and other small things. I'm happy to tighten or loosen any of these rules and go back over the code.

Comment threadpackage.json Outdated

This comment was marked as spam.

@rakyll

Copy link
Copy Markdown
Contributor

This is great, much needed! I left some comments.

@rakyllrakyll added this to the M1: core, datastore & storage milestone Aug 5, 2014
@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

Combing over everything now 💇

@rakyll

Copy link
Copy Markdown
Contributor

Ooops, can't merge automatically. Could you resolve the conflicts?

@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

Just went through it and left in a second commit if you want to see that diff to make sure I'm following our new rules :).

@rakyll

Copy link
Copy Markdown
Contributor

Too many vars :D Please let's do it for constants, enums only, please.

@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

Too many vars :D Please let's do it for constants, enums only, please.

Haha. I think we have to stick to one style. We would run into the issue of having no sensible, detectable style guide. I think the problem we were solving for (this looking weird)...

varsomething='ok',somethingElse='a really long string that goes on forever'+'and ever and ever',anotherThing=3;

...so we instead isolated the long decl...

varanotherThing=3;varsomething='ok';varsomethingElse='a really long string that goes on forever'+'and ever and ever';

...is something that will come up, const or not. Going through the code, I also came across another example of an exception to the rule:

// ...varnamespaceRegex=/^[A-Za-z]*$/;/** * Conversion dict for query operation to operation proto value. * @type {Object} */varopToOperatorDict={'=': 'EQUAL',// ...

So our rule would be, use comma separation for var declarations, unless it's a constant, has a doc block, or is a really long string. I think we're opening the doors for headaches with that! Much more straightforward to use multiple var decls everywhere, imo.

@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

And thank gosh, I finally fixed the merge conflicts! :)

This comment was marked as spam.

This comment was marked as spam.

@rakyll

Copy link
Copy Markdown
Contributor

I think we're opening the doors for headaches with that! Much more straightforward to use multiple var decls everywhere, imo.

I think it'll be pretty straightforward to ask for such tweaks -- giving the fact that it's only a problem for long strings. Styling is about readability, we can't explain the verbosity of unnecessary vars either.

@rakyll

Copy link
Copy Markdown
Contributor

OTOH, could you add yourself to CONTRIBUTORS file?

@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

I think it'll be pretty straightforward to ask for such tweaks -- giving the fact that it's only a problem for long strings. Styling is about readability, we can't explain the verbosity of unnecessary vars either.

It's actually common to use multiple variable declarations. We aren't the exception to the standard here-- the truth is, either way is just as likely to be seen in a JS codebase. Multiple decls is often preferred for the reasons I mentioned earlier, which benefit the browser debugging process, but also for other reasons, such as being able to move a declaration up and down as necessary, without being concerned with leaving a missing comma or ending the decl block on a comma instead of a ;. This PR (JSHint) actually found an issue that would have been avoided by not relying on comma separation of declarations. If I remember right, something like:

vara='b'c='d';

I think there are lots of reasons to support using multiple var decls, but I'm not sure what's in the CONs column for multiple delcs (or the PROs column for comma-separated). You mentioned readability, but as a 90% js dev working in many codebases over the last few years with many different codestyles, multiple var decls doesn't make anything less readable to me, and I don't think others will be slowed down a bit. Again, they're equally common to see. Since we have doc blocks and a hard 80char limit, those are the two reasons I think multiple var decls makes sense for this codebase.

OTOH, could you add yourself to CONTRIBUTORS file?

Yes!

Comment threadpackage.json

This comment was marked as spam.

This comment was marked as spam.

rakyll pushed a commit that referenced this pull request Aug 5, 2014
@rakyll
rakyll merged commit f54daea into googleapis:masterAug 5, 2014
@rakyll

Copy link
Copy Markdown
Contributor

Thanks much!

@stephenplusplus

Copy link
Copy Markdown
ContributorAuthor

Yay!

@jgeewaxjgeewax modified the milestone: M1: core, datastore & storageFeb 2, 2015
sofisl pushed a commit that referenced this pull request Sep 13, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@stephenplusplus@rakyll@jgeewax