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

Import statement for module 2.0 experiment #5890

Closed
wants to merge 2 commits into from
Closed

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Nov 30, 2023

PR Description

This PR implements the import statement defined in this doc. The implementation is based on the current module implementation and differs from the approach tested in #5842.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@wildum wildum requested review from rfratto and thampiotr November 30, 2023 13:07
Copy link
Contributor

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

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

Overall it's looking more surgical, but I agree with the comments re UpdateModuleContent. We can discuss what are alternatives for propagating the imported components' definitions.

}

// Arguments holds values which are used to configure the module.
type Arguments = map[string]any
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this trick also work for exports? Right now I think you'd need to get exports via namespace.comp_name.exports.something instead of namespace.compo_name.something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice one, yes that should work. I tried it with unit tests and it worked :)

@@ -239,6 +244,17 @@ func (l *Loader) Apply(args map[string]any, componentBlocks []*ast.BlockStmt, co
return diags
}

func (l *Loader) onImportContentChange(importLabel string, content string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a few other options for how the updates to contents of an import can be populated and some of them may alleviate many of the issues that you mention in the comments. Let's discuss on our call.

diags.Add(diag.Diagnostic{
Severity: diag.SeverityLevelError,
Message: fmt.Sprintf("Unrecognized component name %q", componentName),
Message: fmt.Sprintf("Explicitly creating a %q in a river file is not allowed", componentName),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could be not adding this component to registry at all. It's already quite special.

@wildum wildum closed this Dec 14, 2023
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants