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

fixing util file names #57

Merged
merged 2 commits into from
May 29, 2024
Merged

fixing util file names #57

merged 2 commits into from
May 29, 2024

Conversation

NooraAz
Copy link
Contributor

@NooraAz NooraAz commented Apr 25, 2024

The argument passed to --class_name will be used as the prefix of util files.

The prefix is the argument passed via class_name. It is added in the description of --class_name.
set a prefix for util files
@NooraAz NooraAz added bug Something isn't working ready_for_review This PR is ready to be reviewed and merged. labels Apr 25, 2024
@roccomoretti
Copy link
Member

This would change the current default behavior for the util files, right? (What happens if you run the script for util without --class_name specified?)

I'm wondering if there's a way you can keep the current behavior with the command lines which currently work, but add in support for the additional behavior if you add in additional options.

@vmullig
Copy link
Member

vmullig commented Apr 28, 2024

I'm wondering if there's a way you can keep the current behavior with the command lines which currently work, but add in support for the additional behavior if you add in additional options.

Since the old behaviour was counter-intuitive (the class name attribute was used to set the file name in all cases except making utility files, and this resulted in util.hh and util.cc being overwritten), I think replacing this with the new behaviour is fine -- especially since this isn't something that came up frequently, and a developer using this functionality is bound to run the generate_templates.py script with --help first. Noora has added a clear description to the --help output about what the new behaviour is. This looks pretty good to me.

Copy link
Member

@vmullig vmullig left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ajasja ajasja merged commit 7e25e4a into RosettaCommons:main May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready_for_review This PR is ready to be reviewed and merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants