Skip to content

Conversation

terrameijar
Copy link
Contributor

This is a proposed fix for #155
I changed the name keyword argument to a positional argument to enable the user to pass in whatever path they like.

@kimberlythegeek
Copy link
Contributor

Thanks for the contribution, @terrameijar !

It looks like the build is failing in flake8 because flake8 runs in python2 instead of 3. I'm opening a PR to clean up the .travis.yml file, and I've specified python 3.6 for the flake8 stage.

This build should pass after those changes are merged.

"""
Write JSON to file with the specified name.
:param name: Name of file to be written to.
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc string should be updated here.

string += "\n\n\n"
return string

def write_results(self, data, name="results.json"):
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a default parameter so that write_results can be called without a filename or path.

:param output: JSON object.
"""
filepath = os.path.join(os.getcwd(), name)
filepath = os.path.abspath(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we set the default value of name to None, we could add a check to write to the current directory when name is undefined.

something like:

def write_results(self, data, name=None):
    filepath = os.path.abspath(name) if name else os.path.join(os.getcwd(), name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I wrote the same over multiple lines because I got a "line to long" error in the linter when I wrote everything on one line.

except NameError:
f.write(json.dumps(data, indent=4))
except FileNotFoundError as fnf_error:
print(fnf_error)

Choose a reason for hiding this comment

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

A better experience would be to create the file if it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section can be removed. @kimberlythegeek suggested that we write to the current directory if there's no path passed to write_results. I intend to do something like open(filepath, 'w') This will make sure that a file is always created, either in the current directory or in the user specified path. I'll submit a new commit now with changes.

@terrameijar
Copy link
Contributor Author

@kimberlythegeek I added another commit to address the changes you requested. Please review when you have a moment.

@kimberlythegeek kimberlythegeek merged commit e85ea45 into django-commons:master Nov 5, 2018
@kimberlythegeek
Copy link
Contributor

thank you @terrameijar !

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.

3 participants