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

add windows and mac support #15

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented Aug 4, 2021

What does this PR do?

Add tests in CI for Windows and macOS to officialize support for them.

Motivation

Windows tests are failing in dd-trace because of a transitive dependency of sketches-js, indicating a potential issue with Windows support.

@rochdev rochdev force-pushed the rochdev/windows-and-mac-support branch from 2600067 to fc2cce4 Compare August 5, 2021 15:05
@rochdev rochdev marked this pull request as ready for review August 5, 2021 16:39
@rochdev rochdev requested a review from brimtown August 5, 2021 16:39
@@ -11,5 +11,13 @@ module.exports = {
'prettier',
'prettier/@typescript-eslint',
'plugin:prettier/recommended'
]
],
rules: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want this here. This prettier/prettier ESLint plugin is reading the settings from the prettier.config.js file, so any formatting-config changes we'd make should go in prettier.config.js

I'm guessing that if you ran into an issue with this, instead of swapping the default value for endOfLine, we should set up a pre-commit hook to run prettier with the project's configuration (or make sure that your local editor is set up to run prettier itself).

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that Windows uses CRLF, and by default Prettier enforces LF only. So the build fails on Windows because of this limitation. I can move the rule to prettier.config.js, but the rule is still needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@brimtown When I add a Prettier config file, it looks line the ESLint config file starts to be ignored. From the information I found online, I think when both Prettier and ESLint are combined then any Prettier configuration should be done in the ESLint configuration. Please let me know if you know of a way to use both.

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.

2 participants