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

ci(SFINT-2577): Ensure Prettier run properly on UNIX. #43

Merged
merged 6 commits into from
Oct 7, 2019
Merged

ci(SFINT-2577): Ensure Prettier run properly on UNIX. #43

merged 6 commits into from
Oct 7, 2019

Conversation

louis-bompart
Copy link
Collaborator

@louis-bompart louis-bompart commented Sep 25, 2019

The glob was not resolved the same way due to escaping on bash vs Powershell, this is now fixed.
Also now using the standard .prettierrc.js, because .json is not standard.

Bash were the ones with degradation, and thus the CI did not trip on linting ('cause he's on Ubuntu)

@louis-bompart
Copy link
Collaborator Author

19acba5 failed as expected.
Now linting the files.

@louis-bompart louis-bompart added the ci & tools All PR that does not affect the product itself but the tools around it label Sep 25, 2019
@louis-bompart louis-bompart changed the title Prettier consistency ongoing ci(SFINT-2577): Ensure Prettier run properly on UNIX. Sep 25, 2019
@louis-bompart louis-bompart marked this pull request as ready for review September 25, 2019 21:03
@@ -1,10 +1,10 @@
*.svg
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably a silly question but what is different here? The changes look the same to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No such thing has a silly question ;P
It's the end of line terminator that have changed: it was a CRLF, it's now a LF, as specified by the .prettierrc.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! Thanks Louis!

@@ -23,8 +23,8 @@
"css": "node ./scripts/css.js",
"dev": "node ./scripts/dev.server.js",
"watchTest": "karma start ./config/karma.config.js",
"lint": "prettier --check **/*",
"lint-fix": "prettier --write **/* ",
"lint": "prettier --check \"./**/*\" --config \".prettierrc.js\" --ignore-path \".prettierignore\"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not targetting the json instead ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I changed it to a .js to be consistent with the other config files

Copy link
Collaborator

Choose a reason for hiding this comment

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

So where is you change to enable linux ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the wrapping of the glob (so **/* to \"./**/*\")

@louis-bompart louis-bompart merged commit 53c94d8 into coveo:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci & tools All PR that does not affect the product itself but the tools around it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants