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 likely project root as fallback cwd #33

Merged
merged 9 commits into from
Mar 4, 2022
Merged

Add likely project root as fallback cwd #33

merged 9 commits into from
Mar 4, 2022

Conversation

remcohaszing
Copy link
Member

@remcohaszing remcohaszing commented Feb 2, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This uses the ancestor directory containng package.json or .git as the cwd for vfile and unified-engine.

Closes #29

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Feb 2, 2022
@remcohaszing remcohaszing marked this pull request as draft February 2, 2022 21:22
cwd = await packageDirectory({
cwd: path.dirname(fileURLToPath(textDocument.uri))
})
}
Copy link
Member

Choose a reason for hiding this comment

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

I recommend switching between the 2 behaviors based on workspacesAsPaths.length > 0, instead of using one as a fallback for both o workspaces existing and a file not in a workspace.

Seems easier to reason about and debug

lib/index.js Outdated Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Feb 3, 2022

  • tests fail
  • what is the reason this is a draft?

@remcohaszing
Copy link
Member Author

This is a draft because the tests are still failing (for good reasons). I just wanted to show it’s being worked on (although my spare time to work on this is a bit limited at the moment)

@codecov-commenter

This comment was marked as resolved.

@remcohaszing remcohaszing marked this pull request as ready for review February 24, 2022 15:12
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @remcohaszing!

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@remcohaszing
Copy link
Member Author

The build is failing because of vfile/vfile-message@ebe7190

test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
@wooorm wooorm changed the title Use directory containing package.json as cwd Add likely project root as fallback cwd Mar 4, 2022
@wooorm wooorm merged commit 02613a6 into unifiedjs:main Mar 4, 2022
@github-actions

This comment has been minimized.

@wooorm wooorm added 💪 phase/solved Post is done 🗄 area/interface This affects the public interface labels Mar 4, 2022
@wooorm wooorm added the 🧒 semver/minor This is backwards-compatible change label Mar 4, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Mar 4, 2022
@remcohaszing remcohaszing deleted the cwd-pkg-dir branch August 6, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change
Development

Successfully merging this pull request may close these issues.

The language server cwd is not a good fallback for unified-engine cwd
5 participants