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

fix: use default node url if the user happens to explicitly set [] #3731

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Apr 2, 2024

Corner case I hit in rules_js. The current error message if you accidentally set node_urls = [] is not great. This bit of code restores the default value if the user happens to clear it with an empty list.

@gregmagolan gregmagolan force-pushed the implicit_default_node_urls branch 2 times, most recently from efc258a to e26ca53 Compare April 2, 2024 05:59
@jbedard
Copy link
Collaborator

jbedard commented Apr 2, 2024

What is the corner case you hit?

Generally "correcting" bad API usage just makes things more complicated imo :/

@gregmagolan
Copy link
Collaborator Author

gregmagolan commented Apr 2, 2024

What is the corner case you hit?

Generally "correcting" bad API usage just makes things more complicated imo :/

The corner case was a wrapper macro. I think the default should be [] personally because that implies that you want to canonical upstream source, however, having the default set to the canonical URL is useful for the doc generator. There is precedent for setting the default within the rule with the node_repositories attribute so this follow a similar pattern. I set the default to [] initially but wasn't happy with the generated docstring with that change.

nodejs/repositories.bzl Show resolved Hide resolved
@gregmagolan gregmagolan enabled auto-merge (squash) April 2, 2024 14:15
@gregmagolan gregmagolan merged commit 6c3170b into main Apr 2, 2024
4 checks passed
@gregmagolan gregmagolan deleted the implicit_default_node_urls branch April 2, 2024 14:15
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