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

Add a way to pass command to the script from a file #34

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

pneumatick
Copy link
Contributor

closes #30

@pneumatick pneumatick requested a review from cpaniaguam July 3, 2024 18:34
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Great start! I noted two good use cases for using standard file formats for configuration files and mutually exclusive groups in a CLI .

Comment on lines 125 to 126

Example: 1, 3 => 1.25
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Example: 1, 3 => 1.25

Comment on lines +176 to +183
"""
Read commands and their arguments from a file.

Expected file format:
- Each command on its own line
- Each line's values are separated by commas (csv file ideal)
- The first value of each line is the command, the rest are its arguments (if any)
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide a sample file that meets these criteria?

Copy link
Member

Choose a reason for hiding this comment

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

Is tests/test_commands.csv an instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was, but I've since replaced it with a file in src/cleanup that I actually used to clean up the latest dataset.

Comment on lines +187 to +197
command_lines = []
with open(filename, 'r') as file:
for line in file:
line = line.rstrip('\n')
command_lines.append(line.split(','))

for line in command_lines:
command = line[0]
columns = line[1:]
if getattr(args, command, None) == None:
setattr(args, command, columns)
Copy link
Member

Choose a reason for hiding this comment

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

Consider using a standard config file format like toml for which Python has native support in the tomllib library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pneumatick Would this be too complicated to add to the project?

src/cleanup/cleanup_script.py Outdated Show resolved Hide resolved
Comment on lines +259 to +262
parser.add_argument(
'--commandfile',
help="Use commands and arguments from a comma separated file. Manually entered commands will override those in the file."
)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good use case for a mutually exclusive group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I could glean from the documentation (which is fairly poor for this specific function) and people trying to use mutually exclusive groups to mimic the behavior of this command, argparse wouldn't be able to deal with it properly. From what I've read there isn't a way to add commands to a group and then add that group to a mutually exclusive group. In other words, every command other than --input, --output, and --commandfile would be placed in a group which would then be added to the mutually exclusive group, and then --commandfile would be added to the mutually exclusive group as well. I don't think that that is possible.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I guess you could add the logic you need and throw exceptions when needed?

if args.foo and (args.bar or args.baz):
    parser.error('--foo cannot be used with --bar or --baz')

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pneumatick Any thoughts on this?

@pneumatick pneumatick changed the base branch from main to cleanup-tests July 5, 2024 15:23
@pneumatick pneumatick marked this pull request as draft July 5, 2024 15:23
@pneumatick pneumatick requested a review from kmilo9999 August 7, 2024 17:28
@pneumatick pneumatick marked this pull request as ready for review August 7, 2024 17:28
Copy link
Collaborator

@kmilo9999 kmilo9999 left a comment

Choose a reason for hiding this comment

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

It's looking good. I left a few comments thought.
We can merge after you address them.

Comment on lines +278 to 282
if args.commandfile != None:
args = commands_from_file(args)
if args.removews != None:
df = remove_whitespace(df, args.removews)
if args.tobool != None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if args.commandfile != None:
args = commands_from_file(args)
if args.removews != None:
df = remove_whitespace(df, args.removews)
if args.tobool != None:
if args.commandfile is not None:
args = commands_from_file(args)
if args.removews is not None:
df = remove_whitespace(df, args.removews)
if args.tobool is not None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while since it happened so I forget exactly what the issue was, but I had tried that initially and it caused the script to fail to execute certain commands. The script does need some slight modifications later anyway so I'll revisit that idea at that point.

Comment on lines +259 to +262
parser.add_argument(
'--commandfile',
help="Use commands and arguments from a comma separated file. Manually entered commands will override those in the file."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pneumatick Any thoughts on this?

Comment on lines +187 to +197
command_lines = []
with open(filename, 'r') as file:
for line in file:
line = line.rstrip('\n')
command_lines.append(line.split(','))

for line in command_lines:
command = line[0]
columns = line[1:]
if getattr(args, command, None) == None:
setattr(args, command, columns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pneumatick Would this be too complicated to add to the project?

@pneumatick pneumatick merged commit 6f1fbb3 into cleanup-tests Aug 7, 2024
@pneumatick pneumatick deleted the modify-cleanup-script branch August 7, 2024 18:13
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.

Separate commands in cleanup_script.py
3 participants