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

Show "did you mean" suggestion for unknown repo names #23184

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 1, 2024

If @boo is parsed in a context in which only @foo is visible, the resulting RepositoryName object will show as follows in error messages:

@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]

If `@boo` is parsed in a context in which only `@foo` is visible, the resulting `RepositoryName` object will show as follows in error messages:

```
@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]
```
@fmeum fmeum requested review from meteorcloudy and Wyverald and removed request for meteorcloudy August 1, 2024 11:53
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 1, 2024
/**
* If ownerRepoIfNotVisible is not null, this field stores the suffix to be appended to the error
*/
@Nullable private final String didYouMeanSuffix;
Copy link
Collaborator Author

@fmeum fmeum Aug 1, 2024

Choose a reason for hiding this comment

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

I could refactor this class into a sealed base class with a single field and a subclass with all three fields. The memory savings wouldn't be that significant (8 bytes per instance, of which there are most likely less than 100,000 even for very large repos).

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks! I'll let @Wyverald answer the memory concern, I guess it should be fine.

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 1, 2024

@bazel-io fork 7.4.0

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 1, 2024

Thanks! I'll let @Wyverald answer the memory concern, I guess it should be fine.

The concern actually isn't new, the new field I added only consumes space previously used for padding. But if we could reduce the field count by two, we could save 8 bytes per instance.

@iancha1992 iancha1992 added the team-CLI Console UI label Aug 2, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 7, 2024

@Wyverald Friendly ping

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 7, 2024
@Wyverald
Copy link
Member

Wyverald commented Aug 7, 2024

Sorry for the delay -- I agree that the memory concern isn't pressing.

@copybara-service copybara-service bot closed this in 868d9f2 Aug 8, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 8, 2024
@fmeum fmeum deleted the unknown-repo-did-you-mean branch August 8, 2024 05:08
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 8, 2024

@bazel-io fork 7.4.0

fmeum added a commit to fmeum/bazel that referenced this pull request Aug 20, 2024
If `@boo` is parsed in a context in which only `@foo` is visible, the resulting `RepositoryName` object will show as follows in error messages:

```
@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]
```

Closes bazelbuild#23184.

PiperOrigin-RevId: 660678414
Change-Id: Iecb5b1d211ff9e1f04ff0f2189376a98b7b261a9
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
If `@boo` is parsed in a context in which only `@foo` is visible, the
resulting `RepositoryName` object will show as follows in error
messages:

```
@@[unknown repo 'boo' requested from @@ (did you mean 'foo'?)]
```

Closes #23184.

PiperOrigin-RevId: 660678414
Change-Id: Iecb5b1d211ff9e1f04ff0f2189376a98b7b261a9

Closes #23189
Closes #23239
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 7.4.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.4.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-CLI Console UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants