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

fix(suggestUtils): use workspace.anchoredLocator #4937

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

paul-soporan
Copy link
Member

What's the problem this PR addresses?

findProjectDescriptors returns a list of workspace.locators, which is then compared against workspace.anchoredLocator, when they can never be equal.

How did you fix it?

Made it return a list of workspace.anchoredLocators since in the future I'd prefer to just completely get rid of workspace.locator anyways and it also makes more sense to display the anchoredLocator in the interactive prompt since it matters more that the package is a workspace, not that it has a specific version.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis
Copy link
Member

arcanis commented Oct 5, 2022

it also makes more sense to display the anchoredLocator in the interactive prompt since it matters more that the package is a workspace, not that it has a specific version.

It probably would make even more sense to use workspace:^ rather than workspace:path/to/foo 🤔

@paul-soporan
Copy link
Member Author

We already do that for selected descriptors (for PROJECT since REUSE has to reuse the exact range that appears in dependencies), this is just for the locator in the (originally used by @yarnpkg/monorepo@0.0.0 and 8 others) message.

@arcanis arcanis closed this Oct 7, 2022
@arcanis arcanis reopened this Oct 7, 2022
@arcanis
Copy link
Member

arcanis commented Oct 7, 2022

(restarting the tests, which didn't trigger due to a GH incident)

@arcanis arcanis merged commit 659d00f into master Oct 7, 2022
@arcanis arcanis deleted the paul/fix/suggestUtils-anchoredLocator branch October 7, 2022 10:31
arcanis pushed a commit that referenced this pull request Jul 28, 2023
**What's the problem this PR addresses?**
<!-- Describe the rationale of your PR. -->
<!-- Link all issues that it closes. (Closes/Resolves #xxxx.) -->

`workspace.locator` is quite useless and only creates confusion with
`workspace.anchoredLocator`.

At the moment, it's only used in places that need an ident, which
`workspace.anchoredLocator` can happily provide.

In the past, it was also used accidentally instead of
`workspace.anchoredLocator` before I fixed it in
#4937.

**How did you fix it?**
<!-- A detailed description of your implementation. -->

Removed it since it doesn't provide any benefits. Its only actual use is
comparisons in `project.tryWorkspaceByLocator`, but nobody has ever
passed it to `project.tryWorkspaceByLocator` anyways.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [X] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [X] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [X] I will check that all automated PR checks pass before the PR gets
reviewed.
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