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

Replace comma in default node names with semicolon #2087

Merged
merged 10 commits into from
Dec 14, 2022

Conversation

jmholzer
Copy link
Contributor

@jmholzer jmholzer commented Nov 30, 2022

Resolves #2013

Description

Development notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@jmholzer jmholzer marked this pull request as ready for review November 30, 2022 19:14
@jmholzer jmholzer requested a review from idanov as a code owner November 30, 2022 19:14
Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

Minor comment - The error that is shown when you try to name a node with a string containing commas is ValueError: 'node, name' is not a valid node name. It must contain only letters, digits, hyphens, underscores and/or fullstops.. I think this could also possibly be modified to add semicolons for consistency.
Otherwise, looks good to me! 🎉

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

This is a breaking change and should go on the develop branch. The ticket is tagged with the 0.19.0 version 🙂

@jmholzer
Copy link
Contributor Author

jmholzer commented Dec 1, 2022

Minor comment - The error that is shown when you try to name a node with a string containing commas is ValueError: 'node, name' is not a valid node name. It must contain only letters, digits, hyphens, underscores and/or fullstops.. I think this could also possibly be modified to add semicolons for consistency. Otherwise, looks good to me! 🎉

Thanks for the review! 😃 I think we might want to leave semicolons from this list, since if Kedro creates default node names with semicolons, that is an implicit statement that these names are correct. Currently, we do it differently; Kedro says that node names with commas are disallowed even though it also creates default node names with commas in them but I feel this is a bug (/ inconsistency) and not a feature.

@jmholzer jmholzer changed the base branch from main to develop December 1, 2022 10:57
@jmholzer
Copy link
Contributor Author

jmholzer commented Dec 1, 2022

This is a breaking change and should go on the develop branch. The ticket is tagged with the 0.19.0 version 🙂

Done 🙂

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! Don't forget to update the release notes to include this change.

@jmholzer jmholzer merged commit 7d6e7d9 into develop Dec 14, 2022
@jmholzer jmholzer deleted the fix/replace-comma-in-default-node-names branch December 14, 2022 22:48
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.

Change autogeneration of node names to not contain commas
3 participants