Skip to content

General cleanup with eslint updates#82

Merged
brody2consult merged 3 commits into
apache:masterfrom
brody2consult:general-cleanup-with-eslint-update
Jul 19, 2019
Merged

General cleanup with eslint updates#82
brody2consult merged 3 commits into
apache:masterfrom
brody2consult:general-cleanup-with-eslint-update

Conversation

@brody2consult
Copy link
Copy Markdown

@brody2consult brody2consult commented Jul 15, 2019

UPDATED after review and discussion:

  • merge changes from PR remove no-throw-literal lint exception not needed #81 to remove standard eslint exceptions from .eslint.yml that are not needed
  • manual cleanup fix for no-prototype-builtins (as needed by new (semi)standard eslint package updates)
  • eslint-config-semistandard@14 & eslint-config-standard@13 updates with automatic cleanup fixes (since the prefer-const & quote-props eslint rules are now enabled)
  • automatic eslint cleanup fixes for padded-blocks that is no longer kept as an exception

I tried to keep these changes split into separate commits in a semi-sensical manner. I think it would be a little too much to put all these changes into a single commit. I am now proposing the most important changes in this PR for discussion, with some apparently controversial changes now split into separate PRs.

My objective is to keep the number of (semi)standard eslint exceptions to a bare minimum.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 15, 2019

Codecov Report

Merging #82 into master will decrease coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   89.04%   89.04%   -0.01%     
==========================================
  Files          20       20              
  Lines        1826     1825       -1     
  Branches      374      373       -1     
==========================================
- Hits         1626     1625       -1     
  Misses        200      200
Impacted Files Coverage Δ
src/CordovaLogger.js 98.46% <ø> (ø) ⬆️
src/util/addProperty.js 100% <ø> (ø) ⬆️
src/ConfigChanges/ConfigChanges.js 94.18% <ø> (ø) ⬆️
src/CordovaError/CordovaError.js 89.28% <100%> (-0.37%) ⬇️
src/util/xml-helpers.js 98.79% <100%> (ø) ⬆️
src/superspawn.js 88.7% <100%> (ø) ⬆️
src/PlatformJson.js 82.69% <100%> (ø) ⬆️
src/FileUpdater.js 86.01% <25%> (ø) ⬆️
src/ConfigParser/ConfigParser.js 78.64% <50%> (ø) ⬆️

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 83954d6...112b5cd. Read the comment docs.

janpio
janpio previously requested changes Jul 15, 2019
Copy link
Copy Markdown
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

same as #81 (review)

@brody2consult brody2consult changed the title General cleanup with eslint updates [WIP] General cleanup with eslint updates Jul 15, 2019
Copy link
Copy Markdown
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

My opinion regarding these changes:

  • ✔️ Re-enable padded-blocks rule
    One minor nitpick: I don't like the generous empty lines you added to the require blocks.
  • ✔️ Re-enable no-throw-literal rule
  • ✔️ quote-props fixes
  • ✔️ (semi)standard eslint updates
  • ❌ Disable no-unused-vars rule
  • ❔ Re-enable operator-linebreak rule
    I think it's good to enforce one style here, although I tend to favor the before style, especially with boolean operators.

I'd also like to remove camelcase: off. But this one will make a lot of noise, so I think we should approach it in a separate PR. Plus we will have to see how many exceptions we have to make for our public interface here.

All in all, this looks very good to me 👍

@raphinesse
Copy link
Copy Markdown
Contributor

@janpio I think consistency with other repos should not be a matter here, if we see this as a test drive towards a better, stricter code style.

@janpio janpio dismissed their stale review July 18, 2019 09:39

I'm ok with it if it is part of a bigger initiative that will be used in a project wide context.

@brody2consult brody2consult force-pushed the general-cleanup-with-eslint-update branch from 8d662bc to 5cbe1f5 Compare July 18, 2019 17:52
@brody2consult brody2consult changed the title [WIP] General cleanup with eslint updates General cleanup with eslint updates Jul 18, 2019
@brody2consult
Copy link
Copy Markdown
Author

I just pushed the more important changes in a different order, in a cleaner sequence, and rebased on master. I moved the seemingly controversial proposals out to separate PRs #85 & #86. I would really appreciate another review.

I pushed the original proposal into my general-cleanup-with-eslint-update-save1 branch in case we want to keep it for comparison..

Copy link
Copy Markdown
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

Approved with my suggested change. Thanks for updating this PR.

Comment thread src/CordovaError/CordovaError.js Outdated
Christopher J. Brody and others added 3 commits July 18, 2019 19:14
as needed for the upcoming (semi)standard eslint package updates

Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Christopher J. Brody <chris@brody.consulting>
- eslint-config-semistandard@14
- eslint-config-standard@13

with src & spec fixed by the following command:

    npm run eslint -- --fix
and remove padded-blocks exception from .eslintrc.yml

with src & spec fixed by the following command:

    npm run eslint -- --fix
@brody2consult brody2consult force-pushed the general-cleanup-with-eslint-update branch from 52f0c0e to 112b5cd Compare July 18, 2019 23:26
@brody2consult brody2consult requested a review from raphinesse July 18, 2019 23:43
@brody2consult
Copy link
Copy Markdown
Author

I applied the change suggested by @raphinesse, with the code alignment fixed, preserved these updates in my general-cleanup-with-eslint-update-save2 branch, then rebased the changes as 3 commits on top of the master branch.

I think these changes should be good to go, would appreciate a final review.

I am not sure whether these changes should be merged by a merge commit or rebase commit (I would not favor a squash merge). The merge commit would clearly show that these changes go together with this PR number, while the rebase commit might be a little less noisy. (GitHub would still show the PR number for each of the commits, though.)

Copy link
Copy Markdown
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update!

I'd merge this as an actual merge. But do as you see fit.

@brody2consult brody2consult merged commit d73d7fa into apache:master Jul 19, 2019
@brody2consult brody2consult deleted the general-cleanup-with-eslint-update branch July 19, 2019 12:27
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