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

Add tests #30

Merged
merged 7 commits into from
Mar 7, 2019
Merged

Add tests #30

merged 7 commits into from
Mar 7, 2019

Conversation

inishchith
Copy link
Collaborator

I've added self-explanatory commit messages which will make this easier to reproduce.

@sauravchirania Can you please set this up locally.
I want you to form a basic structure for the other test and also review this PR and let me know if anything that can be done in a better way.

Footnotes

Note: setup.cfg ignores some flake8 which can be fixed after this merge
Remove .json from gitignore for tests
Update README.md to show build status
Add refresh method to dcli.py for tests
Add test_auth + base
revert .gitignore for development setup
Copy link

@armudgal armudgal left a comment

Choose a reason for hiding this comment

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

Hi thanks for the PR but there are improvements that can be done.

Please go through tox and pytest. Tox will enable us to test linting as well as testing efficiently and pytest will allow us to focus on code coverage as well using pytest-codecov.

P.S. I am not one of the mentors. Feel free to bash me if you think my views are wrong 😛

- pip install flake8

install:
- python3 setup.py install
Copy link

Choose a reason for hiding this comment

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

Hi, setup.py install command is kind of obsolete since pip came along.

@@ -0,0 +1,9 @@
requests
Copy link

Choose a reason for hiding this comment

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

This file becomes redundant because the packages are mentioned in setup.py. If you are concerned about the development setup, populate this file only with -e .[dev], and mention dev dependencies if there are any in setup.py

Copy link
Collaborator

@sauravchirania sauravchirania left a comment

Choose a reason for hiding this comment

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

@inishchith Not familiar with the OAuth part tbh. Looks good to me otherwise :-)

@inishchith
Copy link
Collaborator Author

@armudgal Thanks, really appreciate your time. I'll have a look at this in some time ;)

@nurdtechie98
Copy link
Owner

@inishchith LGTM, apart from the requirements.txt part is it required as all the mentioned dependencies are already taken care of in setup.py

@nurdtechie98 nurdtechie98 merged commit 4367b87 into nurdtechie98:dev Mar 7, 2019
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.

4 participants