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 code completion for chained value factories #11

Merged
merged 7 commits into from
Feb 20, 2025

Conversation

jbartok
Copy link
Member

@jbartok jbartok commented Feb 5, 2025

No description provided.

@jbartok jbartok force-pushed the jb/support-chained-value-factories branch 3 times, most recently from 9a2dffa to 31db1d8 Compare February 5, 2025 12:41
@jbartok jbartok marked this pull request as ready for review February 5, 2025 12:41
@jbartok jbartok force-pushed the jb/support-chained-value-factories branch from 31db1d8 to fd499f9 Compare February 5, 2025 12:44
dataClass: DataClass,
analysisSchema: AnalysisSchema
): List<CompletionItem> {
fun indexValueFactories(analysisSchema: AnalysisSchema, type: DataClass, namePrefix: String): Map<FqName, List<LabelAndInsertText>> {
Copy link
Member

@h0tk3y h0tk3y Feb 10, 2025

Choose a reason for hiding this comment

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

💭 Given that with a big enough schema this might be an expensive operation, I would suggest finding a way to build the index once (or maybe precompute/memoize some parts of it) and invalidate it on schema changes.

For instance, some precomputed or memoized state could include:

  • For each value type, all value factories of that type grouped by the container type.
  • For each container type, the set of paths like layout.settingsDirectory starting from that type (along with the type of layout.settingsDirectory)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've addressed this, please take a look.

return factoryIndex
}

val factories = indexValueFactories(analysisSchema, analysisSchema.topLevelReceiverType, "")
Copy link
Member

Choose a reason for hiding this comment

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

💭 Currently, in addition to the top-level type, you can use the members of the nested containers. For instance, in

foo {
    bar { 
        baz = qux.quux()
    }
}

qux could be a member of:

  • bar's receiver
  • foo's receiver
  • the top-level receiver

We should probably discuss if we want to keep it that way, but if we do, these nested receivers should also contribute the value factory groups to completion.

Copy link
Member Author

Choose a reason for hiding this comment

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

As it works now, qux will be found regardless which of those receivers it's on and will be proposed as a potential value for properties with the same type as the return type of the quux() function.

I think the real problem we should think about is that qux.quux() will be proposed as a potential value for baz even if qux comes from a totally unrelated block, which is part of the top-level receiver, but is unrelated to foo (thus by extension unrelated to bar and baz too).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO.

@jbartok jbartok force-pushed the jb/support-chained-value-factories branch from 2bf0a4b to 6047401 Compare February 14, 2025 14:09
Copy link
Member

@h0tk3y h0tk3y left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jbartok jbartok merged commit c67a99b into main Feb 20, 2025
5 checks passed
@jbartok jbartok deleted the jb/support-chained-value-factories branch February 20, 2025 11:06
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.

3 participants