-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: SP-1856 scanoss py update scanoss settings file schema #84
feat: SP-1856 scanoss py update scanoss settings file schema #84
Conversation
d6fc531
to
12c80b8
Compare
…iles, hidden folders, etc
operation_type: str = 'scanning', | ||
skip_size: int = 0, | ||
skip_extensions = None, | ||
skip_folders = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would add the types here for skip options
also i'm not 100% convinced we need to pass them to the constructor.. it's a little confusing with the other arguments (all_extensions, all_folders, hidden_files_folders
)
maybe we can create setters for these, because we could want to add them after instantiation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, are we sure we want to set operation type as default? from my pov this should be a required argument then
|
||
Returns: | ||
list[str]: Filtered list of files to scan or fingerprint | ||
""" | ||
if self.debug: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we could remove this if check
you can just do self.print_debug
and it will do it
all_files = [] | ||
root_path = Path(root).resolve() | ||
|
||
if not root_path.exists() or not root_path.is_dir(): | ||
self.print_stderr(f'ERROR: Specified root directory {root} does not exist or is not a directory.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be print_debug
?
scan_root (str): Root directory to scan or fingerprint | ||
|
||
Returns: | ||
list[str]: Filtered list of files to scan or fingerprint | ||
""" | ||
filtered_files = [] | ||
for file_path in files: | ||
if not os.path.isfile(file_path): | ||
if not os.path.exists(file_path) or not os.path.isfile(file_path) or os.path.islink(file_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could remove the .exists()
check since both isfile
and islink
already check for existance
return filtered_files | ||
|
||
def _get_file_folder_pattern_spec(self, operation_type: str = 'scanning'): | ||
""" | ||
Get file path pattern specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are using PEP 257 standard for docstrings as it seems to be the one used across new python scripts, as you can see in the other methods from this class and the new ones
return True | ||
|
||
# Look for custom (extra) endings | ||
if self.skip_extensions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe is cleaner to write it just in one line like
if self.skip_extensions and any(file_name_lower.endswith(ending) for ending in self.skip_extensions)
or something like that
skip_size=self.skip_size, | ||
skip_folders=self.skip_folders, | ||
skip_extensions=self.skip_extensions, | ||
operation_type='scanning' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're passing here operation_type = 'scanning', should be 'fingerprinting'
also i think is cleaner just to instantiate it once and pass operation type as argument to the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.