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

chore: use standard #84

Merged
merged 11 commits into from
Jul 27, 2020
Merged

chore: use standard #84

merged 11 commits into from
Jul 27, 2020

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Jul 10, 2020

Switches to using Standard rather than lintit, after noticing that the code style was relatively... different than most modern JavaScript I've seen.

Switching to Standard did require several bits of refactoring outside of the normal standard --fix to make the CLI compliant, including moving to new URL() rather than url.parse(), shifting around some inline functions, and tweaking a few of the RegEx (did my best to validate that they would still do what they should with external tooling in addition to running npm run test).

cc @codebytere

@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #84 into master will decrease coverage by 0.44%.
The diff coverage is 45.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   60.93%   60.49%   -0.45%     
==========================================
  Files          18       18              
  Lines         448      443       -5     
==========================================
- Hits          273      268       -5     
  Misses        175      175              
Impacted Files Coverage Δ
lib/rule.js 100.00% <ø> (ø)
lib/rules/line-after-title.js 100.00% <ø> (ø)
lib/rules/metadata-end.js 100.00% <ø> (ø)
lib/rules/pr-url.js 100.00% <ø> (ø)
lib/rules/reviewers.js 100.00% <ø> (ø)
lib/rules/subsystem.js 100.00% <ø> (ø)
lib/rules/title-length.js 100.00% <ø> (ø)
lib/tap.js 5.26% <ø> (ø)
bin/cmd.js 29.88% <12.00%> (-1.58%) ⬇️
lib/format-tap.js 4.76% <50.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ddbb6d...ad3eafd. Read the comment docs.

lib/rules/fixes-url.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jul 11, 2020

I know it's more work for you as the contributor, but I'd prefer that the refactors go in their own commits, or even their own PRs. I'm talking about the url.parse()/new URL() changes in one commit (or one PR), and the regular expression changes in another commit (or PR), and so on, then all the purely aesthetic changes that can be done with --fix in their own commit/PR.

@bnb
Copy link
Contributor Author

bnb commented Jul 13, 2020

@Trott if you'd like, I can do that.

The challenge there is that I had to incrementally run standard --fix since the main CLI file (and I believe one other file, though that one was just a tiny change so I didn't focus on it too much) is actually so far out of what's expected/acceptable by standard that it messes up the parsing to the point it fails. I had to incrementally apply changes and then fix several times.

If you do want me to do that, are you okay with each step in that process being an incremental commit (including multiple fix commits)?

@Trott
Copy link
Member

Trott commented Jul 13, 2020

The challenge there is that I had to incrementally run standard --fix since the main CLI file (and I believe one other file, though that one was just a tiny change so I didn't focus on it too much) is actually so far out of what's expected/acceptable by standard that it messes up the parsing to the point it fails. I had to incrementally apply changes and then fix several times.

Does running standard --fix and doing nothing else result in a file that no longer parses? Or am I misunderstanding?

@Trott
Copy link
Member

Trott commented Jul 13, 2020

Oh, I see. Parsing errors because it doesn't permit return to exit the CLI script and things like that. In ESLint, you can configure it to permit that. standard/standard#167

@Trott
Copy link
Member

Trott commented Jul 13, 2020

I'm fine with whatever. I'm just less likely to invest the time to review if I can't easily skip over everything that standard auto-fixes and focus on the other things. But that's just me!

@bnb
Copy link
Contributor Author

bnb commented Jul 13, 2020

Oh, I see. Parsing errors because it doesn't permit return to exit the CLI script and things like that. In ESLint, you can configure it to permit that. standard/standard#167

There were a couple things like that, but instead of changing standard I approached it by exiting the process (which is actually what @feross suggested as a workaround in that issue). Can discuss that when I refactor, though. Will take a stab at this tomorrow 👍

@bnb bnb force-pushed the bnb/use-standard branch from 1ea8819 to 05460f0 Compare July 14, 2020 19:14
@bnb
Copy link
Contributor Author

bnb commented Jul 14, 2020

reset to the commit before the initial commit I PR'ed and force pushed the changes 👍

@@ -4,7 +4,7 @@
"description": "Validate the commit message for a particular commit in node core",
"main": "index.js",
"scripts": {
"pretest": "lintit && check-pkg",
"pretest": "standard && check-pkg",
Copy link
Member

Choose a reason for hiding this comment

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

We're either going to want to squash everything into one commit when landing, or else split this into two commits: One commit at the start to remove lintit and one commit at the end to add standard.

@Trott
Copy link
Member

Trott commented Jul 14, 2020

reset to the commit before the initial commit I PR'ed and force pushed the changes 👍

Thanks for doing all that. It made reviewing it a lot easier (for me, at least).

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

looks good to me, but in agreement with @Trott we should probably split the lintit removal into a discrete commit

@feross
Copy link

feross commented Jul 17, 2020

@bnb If you need to silence certain warnings temporarily for this PR, you can add this to the top of a file:

/* eslint-disable no-use-before-define */

More info: https://standardjs.com/#how-do-i-hide-a-certain-warning

@bnb
Copy link
Contributor Author

bnb commented Jul 24, 2020

@Trott / @codebytere is this something I should do or is it something you'd do when merging?

@codebytere
Copy link
Member

On second thought i'm fine just squashing everything into a single commit!

@codebytere codebytere merged commit d0f3d51 into nodejs:master Jul 27, 2020
@bnb bnb deleted the bnb/use-standard branch July 27, 2020 17:29
Sign up for free to 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.

4 participants