Skip to content

Initial structure #1

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.

Sign up for GitHub

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

Jump to bottom
Merged
atodorov merged 1 commit into from
Jan 27, 2019
Merged

Initial structure #1

atodorov merged 1 commit into from
Jan 27, 2019

Conversation

@Stranger6667
Copy link
Contributor

@Stranger6667 Stranger6667 commented Jan 26, 2019

@Stranger6667
Copy link
Contributor Author

Stranger6667 commented Jan 26, 2019 •
edited
Loading

Hello @atodorov !

Could you, pls, check if everything is OK with license, naming, supported Python / pytest versions, etc?
Also, it would be nice to have Travis CI enabled for this repo :)

Thank you

@Stranger6667 Stranger6667 requested a review from atodorov January 26, 2019 16:26
atodorov
atodorov requested changes Jan 26, 2019
View reviewed changes
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Overall looks good, some adjustments are needed or have to be clarified.

tox.ini Outdated
[testenv:flake8]
skip_install = true
deps = flake8
commands = flake8 pytest_kiwitcms setup.py tests
Copy link
Member

@atodorov atodorov Jan 26, 2019

Choose a reason for hiding this comment

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

I also like to enable pylint, see the TAP plugin for options.

Copy link
Contributor Author

@Stranger6667 Stranger6667 Jan 26, 2019

Choose a reason for hiding this comment

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

Done. What do you think about adding also isort and black?

Copy link
Member

@atodorov atodorov Jan 27, 2019

Choose a reason for hiding this comment

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

I like the idea of black but never used it. If you are happy with it (or isort) feel free to include them. I don't have an opinion as long as pylint and flake8 are happy.

@atodorov
Copy link
Member

atodorov commented Jan 26, 2019

@Stranger6667 thanks for your effort. A few notes:

  • Travis CI enabled
  • LICENSE is good, for the other plugins we also prefer GPLv3 or later
  • I am finishing work on https://github.com/kiwitcms/tap-plugin/. It is implemented in Python and there are a few important things to note here:
    • The Plugin class contains several get_xxxx_id() methods which are solely concerned about reading configuration from the environment and matching that up to the database. The same methods will be used by the junit.xml plugin and you can use them too. My plan is to move them under tcms_api package as helpers.
    • There are a few new backend API methods without which the plugin will not be able to work (CC @apetkova, FYI).
    • I am going to update the demo instance with a custom built docker image, most likely tomorrow b/c I need to work on integrating the TAP plugin with the live demo and see how that works.
    • There is also a test suite, which mostly covers the helper methods so it will be moved as well. A lot of the behavior spec will be covered for you if you write in Python.

@Stranger6667 Stranger6667 force-pushed the initial-structure branch 2 times, most recently from 71667dc to 51aa989 Compare January 26, 2019 20:51
atodorov
atodorov reviewed Jan 27, 2019
View reviewed changes
.travis.yml

cache:
directories:
- $HOME/.cache/pip
Copy link
Member

@atodorov atodorov Jan 27, 2019

Choose a reason for hiding this comment

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

cache: pip will do the same I believe.

tox.ini Outdated
[testenv:flake8]
skip_install = true
deps = flake8
commands = flake8 pytest_kiwitcms setup.py tests
Copy link
Member

@atodorov atodorov Jan 27, 2019

Choose a reason for hiding this comment

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

I like the idea of black but never used it. If you are happy with it (or isort) feel free to include them. I don't have an opinion as long as pylint and flake8 are happy.

@atodorov
Copy link
Member

atodorov commented Jan 27, 2019

I will take a fresh look at this tomorrow but IMO it is generally OK to merge, even as-is.

For anything that remains I will open issues or pull requests.

@atodorov atodorov merged commit 70406a7 into master Jan 27, 2019
@atodorov atodorov deleted the initial-structure branch January 27, 2019 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atodorov atodorov atodorov requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Stranger6667 @atodorov