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.

Sign up for GitHub

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

Jump to bottom

Run recommended config against source code #387

Merged
merged 38 commits into from
Oct 12, 2019
Merged

Run recommended config against source code #387

merged 38 commits into from
Oct 12, 2019

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Sep 22, 2019 •
edited
Loading

Shouldn't we run recommended configs on our own repo? And our own source code should follow recommended rules.

This is a prototype, maybe move to integration test

@fisker fisker changed the title Test on our own source code [WIP] Test on our own source code Sep 22, 2019
@fisker
Copy link
Collaborator Author

fisker commented Sep 22, 2019

/cc @sindresorhus what do you think?

@sindresorhus
Copy link
Owner

Makes sense to me. You need to add configs to files in package.json.

@fisker
Copy link
Collaborator Author

fisker commented Sep 22, 2019

Just a idea, maybe a better way, will do it later

@fisker
Copy link
Collaborator Author

fisker commented Sep 23, 2019 •
edited
Loading

This PR is almost done.

Some source code not following recommended configs, most of them are fixed, some questions:

  1. prevent-abbreviations default checkProperties: true is annoying, I override it for now, maybe we should consider to change that, prevent-abbreviations default option is not good #271 prevent-abbreviations is too eager #367

  2. prevent-abbreviations also complain about get-docs-url.js filename, I can rename it to fix that, and I suggest we do this to follow recommended config, just lots of files, should I do that?

  3. some comments fails on expiring-todo-comments rule, I can add eslint-disable but xo will reports another error

  4. consistent-function-scoping seems buggy? Is this a consistent-function-scoping bug? #391

/cc @sindresorhus

@fisker fisker mentioned this pull request Sep 24, 2019
Closed
This was referenced Sep 24, 2019
Closed
Merged
@fisker fisker changed the title [WIP] Test on our own source code Run recommended config against source code Oct 3, 2019
@fisker fisker marked this pull request as ready for review October 3, 2019 14:23
@sindresorhus
Copy link
Owner

sindresorhus commented Oct 8, 2019 •
edited
Loading

Can you fix the merge conflict?

@fisker
Copy link
Collaborator Author

fisker commented Oct 8, 2019 •
edited
Loading

lint.js can't pass xo, because I'm not using ava?

local test passed

futpib
futpib suggested changes Oct 8, 2019
View reviewed changes
rules/catch-error-name.js Outdated Show resolved Hide resolved
@fisker
Copy link
Collaborator Author

fisker commented Oct 8, 2019

@futpib any idea why CI fails?

@fisker fisker force-pushed the run-on-self branch 2 times, most recently from 04e599a to a3ae40e Compare October 10, 2019 01:58
@fisker fisker mentioned this pull request Oct 10, 2019
Closed
@fisker
Merge remote-tracking branch 'remotes/upstream/master' into run-on-self
d49a17b 
# Conflicts:
#	rules/no-for-loop.js
#	rules/no-process-exit.js
#	rules/prefer-exponentiation-operator.js
#	rules/prefer-reflect-apply.js
#	rules/prefer-spread.js
@fisker
Copy link
Collaborator Author

fisker commented Oct 12, 2019

How the same code success on my CI?

https://travis-ci.com/fisker/eslint-plugin-unicorn/builds/131622273

fisker
fisker commented Oct 12, 2019
View reviewed changes
.travis.yml
Comment on lines +6 to +7
cache:
npm: false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI finally passed, should I keep this?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, for now.

@fisker fisker requested review from sindresorhus and futpib October 12, 2019 10:38
@sindresorhus sindresorhus merged commit d638e54 into sindresorhus:master Oct 12, 2019
@fisker fisker deleted the run-on-self branch October 13, 2019 08:12
@fisker fisker mentioned this pull request Oct 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

@futpib futpib Awaiting requested review from futpib

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fisker @sindresorhus @futpib