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

use pathlib.Path to represent file system paths instead of strings #1534

Closed
williballenthin opened this issue Jun 12, 2023 · 9 comments · Fixed by #1623
Closed

use pathlib.Path to represent file system paths instead of strings #1534

williballenthin opened this issue Jun 12, 2023 · 9 comments · Fixed by #1623
Assignees
Labels
breaking-change introduces a breaking change that should be released in a major version enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@williballenthin
Copy link
Collaborator

throughout the codebase, replace string type variables with pathlib.Path type variables that represent file system paths. these objects have convenient methods/properties for common operations, as well as make clear how the data should be used (as a file system path, not a string to display to the user, or whatever).

@williballenthin williballenthin added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed breaking-change introduces a breaking change that should be released in a major version labels Jun 12, 2023
@williballenthin
Copy link
Collaborator Author

@Aayush-Goel-04 are you interested in taking a stab at this? we'll want to replace any uses of os.path with pathlib.

@Aayush-Goel-04
Copy link
Contributor

Yes, will give it a try.
Thanks @williballenthin

@Aayush-Goel-04
Copy link
Contributor

@williballenthin
Should I replace os.remove(path) with path.unlink() or make it os.remove(str(path)) if path is pathlib.Path type variable

@williballenthin
Copy link
Collaborator Author

yeah let's prefer to use pathlib where possible, so that means path.unlink.

@Aayush-Goel-04
Copy link
Contributor

@williballenthin
In capa/main should function like get_rules(rule_paths) / get_signatures(sigs_path) recieve paths of type strings, or pathlib.Path.
Currently it is passed directly as type strings from args.rules and args.signatures

@williballenthin
Copy link
Collaborator Author

we should try to work with pathlib as much as possible.

we'll want to convert from str to pathlib.Path as soon as possible, and convert back to str only when we're about to render it to a user.

@Aayush-Goel-04
Copy link
Contributor

Aayush-Goel-04 commented Jun 24, 2023

Hey @williballenthin ,
I was planning to change type from str to path for rules, signatures & samples in install common args.
What should I change their default values to ? Current default [ 'embedded rules' ] , 'embedded signatures' of type str.
Then handle_common_args would need to be updated.

@williballenthin
Copy link
Collaborator Author

please propose something that makes sense to you and we can discuss. i think it just needs to be sensible; i don't think we have to be too strict with these types, but it's something to try for.

for example, it's ok to use str with argparse right when the CLI args come in. we'd just want to convert to pathlib before any other code works with the data.

hope that makes sense, happy to talk further. but remember, i trust you to figure out a good solution :-)

@mr-tz
Copy link
Collaborator

mr-tz commented Jul 12, 2023

Thanks, @Aayush-Goel-04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change introduces a breaking change that should be released in a major version enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
3 participants