Skip to content
This repository has been archived by the owner on Oct 5, 2019. It is now read-only.

adds other chrome profile directories to the collection list #154

Merged
merged 8 commits into from
Aug 21, 2017

Conversation

ytonui
Copy link
Contributor

@ytonui ytonui commented Aug 8, 2017

No description provided.

Copy link
Contributor

@jjsendor jjsendor 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! I think the important thing to fix would be to squeeze these 3 loops into 1.

profile_paths = []
for subdir in os.listdir(chrome_path):
if os.path.isdir(os.path.join(chrome_path, subdir)):
if os.path.isfile("{0}/{1}/History".format(chrome_path, subdir)):
Copy link
Contributor

Choose a reason for hiding this comment

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

This if statement could be merged with the one in the line before using and.

if not os.path.isdir(chrome_path):
Logger.log_warning('Directory not found {0}'.format(chrome_path))
return

profile_paths = []
for subdir in os.listdir(chrome_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a lengthy list comprehension, but you should be able to squeeze it into one if you'd like to :) In a Pythonic way ;)

E.g.:

profile_paths = [pathjoin(chrome_path, subdir) for subdir in os.listdir(chrome_path) if os.path.isdir(os.path.join(chrome_path, subdir)) and os.path.isfile("{0}/{1}/History".format(chrome_path, subdir))]

or sth like that ;)


with Logger.Extra('osxcollector_subsection', 'preferences'):
self._log_json_file(chrome_path, 'preferences')
for profile_path in profile_paths:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you have introduced 3 new for loops, each looping over the same resource. Any way you can refactor this code to merge it into 1 loop?

@ytonui
Copy link
Contributor Author

ytonui commented Aug 15, 2017

I've updated as per review comments

Copy link
Contributor

@jjsendor jjsendor left a comment

Choose a reason for hiding this comment

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

Try to cut this long line ;)

Maybe it was actually more readable when it was for/if? It's up to you to decide, I am just suggesting ;)

if os.path.isdir(os.path.join(chrome_path, subdir)):
if os.path.isfile("{0}/{1}/History".format(chrome_path, subdir)):
profile_paths.append(pathjoin(chrome_path, subdir))
profile_paths = [pathjoin(chrome_path, subdir) for subdir in os.listdir(chrome_path) if os.path.isdir(os.path.join(chrome_path, subdir)) and os.path.isfile("{0}/{1}/History".format(chrome_path, subdir))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, okay, so now the tricky question is how do you split it into multiple lines so it is readable? ;)

@jjsendor
Copy link
Contributor

@ytonui for some reason Travis is failing :(
It complains about the lack of Foundation package, which is weird, because we have the Travis profile set up to build like it was an macOS environment.

@jjsendor
Copy link
Contributor

👍 shipit!

@ytonui ytonui merged commit 2581ee5 into master Aug 21, 2017
@ytonui ytonui deleted the adds_chrome_paths_issues/153 branch August 21, 2017 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants