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

chore: tweak azure-pipelines.yml #5498

Merged
merged 7 commits into from Nov 17, 2018

Conversation

ikatyang
Copy link
Member

  • add missing AST_COMPARE
  • test on Node 4/10

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

Try the playground for this PR

@j-f1
Copy link
Member

j-f1 commented Nov 16, 2018

Can you move the new files to a .azure-pipelines directory?

@kachkaev
Copy link
Member

kachkaev commented Nov 16, 2018

Can there be two azure pipelines with tests? One with git config --global core.autocrlf true (checks out files with CRLF by default) and git config --global core.autocrlf false (checks out files as is).

If the repo’s gtattributes are configured correctly, that should not affect tests as all files will be checked out equally regardless of the global setting.

@j-f1
Copy link
Member

j-f1 commented Nov 16, 2018

Netlify is segfaulting :/

@ikatyang
Copy link
Member Author

@kachkaev I'd like to use eol=lf in #5494 and add another test flag TEST_CRLF to test CRLF by replacing LF with CRLF before formatting. We shouldn't need to configure global settings and we should be able to test CRLF locally even if it's on LF system (via TEST_CRLF).

@j-f1 It looks like a random error, I've rebuilt it and it's green now.

I'm going to merge this PR now since it's only for tweaking the current settings, CRLF should be handled in #5494.

@ikatyang ikatyang merged commit 85eb8cb into prettier:master Nov 17, 2018
@ikatyang ikatyang deleted the chore/azure-pipeline-tweak branch November 17, 2018 04:38
@kachkaev
Copy link
Member

kachkaev commented Nov 17, 2018

We shouldn't need to configure global settings and we should be able to test CRLF locally even if it's on LF system (via TEST_CRLF).

Testing on Windows machines with globally configured CRLF and LF in git is what I meant. This can be useful because Windows folks have different preferencess and all of them should be able to contribute to Prettier without problems of running local tests. By default, git’s core.autocrlf is true, but some devs set it to false, e.g. create-react-app contribution guidelines recommend doing so.

I did not know Azure pipelines had a flag for CRLF, interesting!

@kachkaev
Copy link
Member

Running tests on a Windows machine with LF ia different than doing this on Linux, because on Windows os.EOL is CRLF regardless of how git is configured. Some modules refer to this value and this can affect what Prettier generates in the end.

@ikatyang
Copy link
Member Author

@kachkaev Can you move your comments to #5494? It should be the better place for CRLF discussion.

@ikatyang ikatyang mentioned this pull request Nov 17, 2018
1 task
@ikatyang
Copy link
Member Author

(TEST_CRLF is a new flag I added in our run_spec.js. 😅)

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants