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

Walk hierarchy to add root modules #176

Merged
merged 6 commits into from
Jun 24, 2020
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jun 22, 2020

This aims to solve some scenarios outlined in #32


Because detection of the relevant root module for a file/dir is a non trivial task, I also introduce candidate finder which can report all potential candidates for root modules in the context of a directory being initialized/opened.

The following factors may play role in deciding about the right root module:

Whether the root module was initialized at all (REQUIREMENT)

This is a hard requirement, at least for now. i.e. Root module has to be initd before it's even considered as a candidate (i.e. it has to have .terraform). The module cache provides enough details about module hierarchy in a very efficient and simple way.

Plugin Version Proximity (NOT IMPLEMENTED)

#180

Hierarchy Proximity (NOT IMPLEMENTED)

#179


Before the above detection is implemented we just return the first positive match (in whatever order filepath.Walk finds it) and display a warning in case there is more than 1 match.

The only awkward thing is that every IDE implements windows/showMessage differently. I assume they just work within the constraints of the client API 🤷

VSCode

Screenshot 2020-06-24 at 07 39 38

Sublime Text

Screenshot 2020-06-24 at 07 47 37

NeoVim

Screenshot 2020-06-24 at 07 47 04

@radeksimko radeksimko added the enhancement New feature or request label Jun 22, 2020
@FernandoMiguel

This comment has been minimized.

@radeksimko
Copy link
Member Author

@FernandoMiguel Just to be clear the example I mentioned above isn't assuming any particular filenames. It's just an example.

I will lock this thread temporarily until we iron out the implementation details as it's not quite ready yet for wider public consumption.

Generally though feedback related to different hierarchies is welcomed in #32

Thanks.

@hashicorp hashicorp locked as off-topic and limited conversation to collaborators Jun 22, 2020
@radeksimko radeksimko added this to the v0.4.0 milestone Jun 22, 2020
@radeksimko radeksimko force-pushed the f-root-module-discovery branch 4 times, most recently from 66db19e to a40c1c9 Compare June 23, 2020 16:22
Because detection of the relevant root module for a file/dir
is a non trivial task, we also introduce candidate finder
which can report all potential candidates for root modules
in the context of a directory being initialized/opened.
@radeksimko radeksimko force-pushed the f-root-module-discovery branch from a40c1c9 to 6c152a3 Compare June 23, 2020 19:15
This turns lookups from O(1) to O(n), but the n is in most cases so low
that it likely doesn't matter anyway.
@radeksimko radeksimko marked this pull request as ready for review June 24, 2020 06:53
@hashicorp hashicorp unlocked this conversation Jun 24, 2020
@radeksimko
Copy link
Member Author

This should provide some details about what kind of hierarchies will be supported when this PR is merged:
https://github.com/hashicorp/terraform-ls/tree/f-root-module-discovery/internal/terraform/rootmodule/testdata

Co-authored-by: Paul Tyng <ptyng@hashicorp.com>
}

func renderCandidate(rootDir, path string) string {
return strings.TrimPrefix(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is a better filepath func to do this to get the relative path? String manipulation feels wrong but maybe its fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it does not feel quite right, but I couldn't find filepath.TrimPrefix or anything similar.

@radeksimko radeksimko merged commit 186247c into master Jun 24, 2020
@radeksimko radeksimko deleted the f-root-module-discovery branch June 24, 2020 13:40
@ghost
Copy link

ghost commented Jul 24, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants