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

Adapt new ESLint and Prettier configuration #6284

Merged
merged 10 commits into from Jun 26, 2019

Conversation

Projects
2 participants
@medikoo
Copy link
Member

commented Jun 20, 2019

It covers following tasks, covered with individual commits:

  • 346faf7 - New ESlint & Prettier config integration
  • 72ba232 - Travis config fix and cleanup:
    • Ensure to take some git history, to avoid issues where tested commit is not found
    • Separate PR and branch lint/test job configuration
  • 4e2ed4d - Confirm changed files against Prettier in CI
  • f98b90e - Configure test-ci a command that runs all validation checks that are pursued in CI
  • 81b419b - Fix .editorconfig configuration for markdown files (so trim of ending whitespace is not pursued)
  • 2a870d0 - Educate on pursued validation checks in PR template
  • f46dc03 - Add .js extension to bin/serverless so it's properly covered by ESLint and Prettier
  • 4944f47 - (automatic) Prettify all files
  • e6ae898 - Fix lint issues surfaced after automatic prettification

Compare all changes aside of automatic prettification: master...583197a


Is this ready for review?: YES
Is it a breaking change?: NO

@medikoo medikoo self-assigned this Jun 20, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from ba8278d to fb6eacf Jun 20, 2019

@medikoo medikoo added this to In progress in Serverless via automation Jun 20, 2019

@medikoo medikoo added this to the 1.46.0 milestone Jun 20, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch 2 times, most recently from 0a1c369 to 4c9edbf Jun 20, 2019

@pmuens pmuens added the enhancement label Jun 21, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch 6 times, most recently from 4f0d333 to efbbdd9 Jun 21, 2019

@medikoo medikoo marked this pull request as ready for review Jun 24, 2019

@medikoo medikoo added pr/in-review and removed pr/in-progress labels Jun 24, 2019

@medikoo medikoo requested a review from pmuens Jun 24, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from efbbdd9 to df9c651 Jun 24, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from df9c651 to bc128d5 Jun 24, 2019

@pmuens
Copy link
Member

left a comment

This looks good 👍 +1 for the in-depth TOC of changes in the PR description.

I'd say it's GTM, but I'm concerned by all the files re-formattings which will result in a lot of conflicts. There's no perfect timing to introduce this however. Maybe we can discuss this in our meeting today.

"serverless": "./bin/serverless",
"slss": "./bin/serverless",
"sls": "./bin/serverless"
"serverless": "./bin/serverless.js",

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 25, 2019

Member

Might not be a huge difference, but why don't we stick to the ./bin/serverless file instead?

If we're using the .js file here, what is the ./bin/serverless file used for?

This comment has been minimized.

Copy link
@medikoo

medikoo Jun 25, 2019

Author Member

Might not be a huge difference, but why don't we stick to the ./bin/serverless file instead?

File type discoverability is more complicated for many processors, e.g. ESLint, Prettier won't pick it and that means we're not guarded against errors as we should be.

I think it's a good reason to have extension on binary, and most projects I know switched to that (you may check: ESLint, Webpack, Prettier, npm etc.)

If we're using the .js file here, what is the ./bin/serverless file used for?

It's kept to not accidentally break things for those which by chance refer to bin/serverless directly.

It's to be removed with next breaking version release. I explained that in a file:

// TODO: Remove with file with next major (breaking) release
// Left as a fallback placeholder to not accidentally break things
// https://github.com/search?q=%22bin%2Fserverless%22&type=Code

This comment has been minimized.

Copy link
@pmuens

pmuens Jun 25, 2019

Member

File type discoverability is more complicated for many processors, e.g. ESLint, Prettier won't pick it and that means we're not guarded against errors as we should be.

Yep, I'm in the same boat. Was just wondering why we have such files but still keep the old one. However your comment regarding potential breaking changes makes sense (haven't seen the comment).

@medikoo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

I'd say it's GTM, but I'm concerned by all the files re-formattings which will result in a lot of conflicts. There's no perfect timing to introduce this however. Maybe we can discuss this in our meeting today.

Sure, the alternative I see. Is that we do not validate prettier, but we allow to work with prettier if one wants to, but that would result with constant white-space noise in PR changes and git history, which seems quite dirty, I don't think I like that approach.

As Prettier adoption is really huge now, I wouldn't be much afraid of that change, and if developer of some existing PR struggles with conflicts, we may also help (I definitely can, if needed)

@pmuens

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Sure, the alternative I see. Is that we do not validate prettier, but we allow to work with prettier if one wants to, but that would result with constant white-space noise in PR changes and git history, which seems quite dirty, I don't think I like that approach.

As Prettier adoption is really huge now, I wouldn't be much afraid of that change, and if developer of some existing PR struggles with conflicts, we may also help (I definitely can, if needed)

Yes, I can see that and I'm all in for making the change now (given that it's trivial to prettify files nowadays) rather than having this loose "contract" which doesn't enforce prettified files.

@medikoo medikoo modified the milestones: 1.46.0, 1.47.0 Jun 26, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from bc128d5 to fc5f19c Jun 26, 2019

medikoo added some commits Jun 20, 2019

medikoo added some commits Jun 20, 2019

Configure test-ci script
It covers all checks needed for CI to pass

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from fc5f19c to 93bfd5f Jun 26, 2019

@pmuens
Copy link
Member

left a comment

Just did another reivew based on the diff-link you provided.

LGTM :shipit:

So happy that this finally landed in master 🙏

Serverless automation moved this from In progress to Reviewer approved Jun 26, 2019

medikoo added some commits Jun 24, 2019

@medikoo medikoo force-pushed the adapt-new-eslint-and-prettier branch from 93bfd5f to e6ae898 Jun 26, 2019

Serverless automation moved this from Reviewer approved to Needs review Jun 26, 2019

Serverless automation moved this from Needs review to Reviewer approved Jun 26, 2019

@pmuens

pmuens approved these changes Jun 26, 2019

@medikoo medikoo merged commit f93b27b into master Jun 26, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (medikoo) No new issues
Details

Serverless automation moved this from Reviewer approved to Done Jun 26, 2019

@medikoo medikoo deleted the adapt-new-eslint-and-prettier branch Jun 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.