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

Completions for unexported symbols #892

Merged
merged 18 commits into from
May 21, 2021
Merged

Completions for unexported symbols #892

merged 18 commits into from
May 21, 2021

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Feb 13, 2021

@pfitzseb, as we discussed the other day - is this what you meant?

EDIT - a description
Completions are now provided for symbols that are not exported by modules in the current namespace. These are only triggered when the partial string has length > 3. Two modes are provided where:

  • Completion of an unexported variable var from some module m results in the automatic inclusion of a using statement using m: var. This is included at the start of the current root tree and will be attached to existing using m:... if possible; or
  • the completion results in a qualified variable (e.g. m.var)

@ZacLN ZacLN marked this pull request as ready for review February 14, 2021 10:53
@ZacLN ZacLN self-assigned this Feb 14, 2021
@ZacLN ZacLN modified the milestones: Next Patch, Next Minor Feb 14, 2021
@pfitzseb
Copy link
Member

Almost. What I had in mind would make this work for all symbols, not only unexported ones (which may or may not be a good idea :P). Basically to counteract people using using Foo and then not knowing where a symbol comes from.

This feels pretty good though, so I think we could get this in as-is (although it still needs a setting in the UI, right?).

@ZacLN
Copy link
Contributor Author

ZacLN commented Feb 15, 2021

That feels a bit much for me personally but no reason to not include it as an option. Would we also want a project level action that applies that style retrospectively?

Agree on merging this as is for now

@oppo-source oppo-source removed the request for review from a team April 16, 2021 07:35
@davidanthoff davidanthoff requested a review from pfitzseb May 12, 2021 16:02
Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

Needs a review from @pfitzseb and a merge conflict resolution.

pfitzseb
pfitzseb previously approved these changes May 20, 2021
Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Alright, let's get this in!

Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

Tests fail. Do we maybe need a tag for some dependency for this to work?

@davidanthoff davidanthoff merged commit 2a3e258 into master May 21, 2021
@davidanthoff davidanthoff deleted the comps branch May 21, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants