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

Adds Pipfile support #25

Merged
merged 12 commits into from
Jun 28, 2018
Merged

Adds Pipfile support #25

merged 12 commits into from
Jun 28, 2018

Conversation

tomdottom
Copy link
Collaborator

Addresses #18 and implements some of #10

Current usage:

thanks package_name
thanks -r requirements.txt
thanks -p Pipefile
thanks package_name -r requirements.txt -r test_requirements.txt -p Pipfile

tomdottom added 7 commits May 20, 2018 23:59
Corrected linting errors and removed rouge 'print' calls.

Moved to using nosetest as the setup.py test runner. This is mainly as nose
give clearer error messages when something goes wrong.

Also of note there were pinned dependencies which have been changed to
'at least' version dependencies. For reasons why see
https://caremad.io/posts/2013/07/setup-vs-requirement/

Laster, we currently have a README.md and a README.rst. I'm unsure which one of these
we mean to use. Shouldn't we at the very least keep all of the following in
the same markup format: README, HISTORY, CONTRIBUTING, AUTHORS,
@tomdottom
Copy link
Collaborator Author

🤔 failing tests. But tox passes locally 😭

Will have to return to this when I'm not so 😴

Copy link
Owner

@phildini phildini left a comment

Choose a reason for hiding this comment

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

Overall I think this is amazing, thank you. I have a couple small comments, but I think this is mostly ready-to-ship.

thanks/cli.py Outdated
@click.option("--pipfile", "-p",
multiple=True,
type=click.File("r"))
# @click.option("--setuppy", "-s",
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's good to support these in the future, but would rather not have commented out code lying around. If we want to add a # TODO: Support more types I'm fine with that.

thanks/thanks.py Outdated

return '\n'.join(lines)

def rocks(self, colored_output=True):
Copy link
Owner

Choose a reason for hiding this comment

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

could we instead call this give or print? I was confused reading this what rocks was referring to.

- thanks.rocks is confusing
- removes commented out code
@tomdottom tomdottom force-pushed the thanks-10 branch 2 times, most recently from 8f72a81 to 35e730b Compare June 27, 2018 20:33
- Moves to py.test as gives better error messages
.egg files are sensibly added to .gitignore
This is probably one of the very few cases where you want to
add a .egg file :)
@tomdottom
Copy link
Collaborator Author

Ok @phildini, anything else?

@phildini phildini merged commit 4059095 into phildini:master Jun 28, 2018
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.

2 participants