Skip to content

Use relative paths with CLI#2969

Merged
azz merged 3 commits intoprettier:masterfrom
ahmedelgabri:patch-1
Oct 4, 2017
Merged

Use relative paths with CLI#2969
azz merged 3 commits intoprettier:masterfrom
ahmedelgabri:patch-1

Conversation

@ahmedelgabri
Copy link
Copy Markdown
Contributor

@ahmedelgabri ahmedelgabri commented Oct 3, 2017

Fixes #2964

@ikatyang I need some pointers for the tests. I thought about adding another test inside tests_integration/__tests__/ignore-absolute-path.js

@ikatyang
Copy link
Copy Markdown
Member

ikatyang commented Oct 3, 2017

I think you can add them in a new file tests_integration/__tests__/ignore-relative-path.js and add their fixtures in tests_integration/cli/ignore-relative-path.

ikatyang
ikatyang reviewed Oct 3, 2017
View reviewed changes
tests_integration/__tests__/ignore-relative-path.js Outdated
describe("support relative paths", () => {
runPrettier("cli/ignore-relative-path", [
path.resolve(__dirname, "../cli/ignore-relative-path/level1/level2/level3/file.js"),
path.resolve(__dirname, "../cli/ignore-relative-path/anotherfile.js"),
Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 3, 2017

Choose a reason for hiding this comment

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

These should be relative paths, path.resolve() will make them absolute.

Copy link
Copy Markdown
Contributor Author

@ahmedelgabri ahmedelgabri Oct 3, 2017

Choose a reason for hiding this comment

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

Yes I know, I just wanted to push the changes & will clean it up the whole PR in an hour or two 😊

Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 3, 2017

Choose a reason for hiding this comment

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

Let me know if you're ready. 👍

tests_integration/cli/ignore-relative-path/anotherfile.js
@@ -0,0 +1,2 @@
var x = 'this should be formatterd';

Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 3, 2017

Choose a reason for hiding this comment

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

Name this file shouleBeFormat.js would be better.

Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 3, 2017

Choose a reason for hiding this comment

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

Or shouldNotBeIgnored?

tests_integration/cli/ignore-relative-path/level1/level2/level3/file.js
@@ -0,0 +1 @@
var x = 'this should not be formatterd';
Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 3, 2017

Choose a reason for hiding this comment

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

Name this file shouleNotBeFormat.js would be better.

@ahmedelgabri
Copy link
Copy Markdown
Contributor Author

ahmedelgabri commented Oct 3, 2017

@ikatyang done, updated the tests & rebased. You can check it now

@lydell
Copy link
Copy Markdown
Member

lydell commented Oct 3, 2017

@ahmedelgabri You have a minor lint error that causes the tests to fail :)

  ✘  no-unused-vars  'path' is assigned a value but never used  

  tests_integration/__tests__/ignore-relative-path.js:3:7

  const path = require("path");

         ^

@ahmedelgabri
Copy link
Copy Markdown
Contributor Author

ahmedelgabri commented Oct 3, 2017

Oops, @lydell should be fixed now :)

@azz azz merged commit 2fed6c8 into prettier:master Oct 4, 2017
@azz
Copy link
Copy Markdown
Member

azz commented Oct 4, 2017

Thanks!

@ahmedelgabri ahmedelgabri deleted the patch-1 branch October 5, 2017 20:52
@BPScott BPScott mentioned this pull request Nov 21, 2018
Closed
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ikatyang ikatyang ikatyang approved these changes

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahmedelgabri @ikatyang @lydell @azz