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 support for Draft flag on GitHub #126

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hluk
Copy link
Contributor

@hluk hluk commented Jun 21, 2022

No description provided.

reviewrot/githubstack.py Outdated Show resolved Hide resolved
@hluk
Copy link
Contributor Author

hluk commented Aug 24, 2022

@pbortlov Please review.

web/js/site.js Outdated Show resolved Hide resolved
Comment on lines +40 to +45
[testenv:black-format]
skip_install = true
deps =
black
commands =
black reviewrot/ test/

Choose a reason for hiding this comment

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

So for context, this project previously didn't have any flake8 or black linting.
I wrote a previous patch to PEP8 bomb the project, and added these linters to Tox.

My concern here is how this black-format would auto format, and I'm not 100% sure that's the best approach.

I'd rather see folks use Tox locally, prior to pushing, and amend their commits if necessary. That way all code changes are tracked by a commit end-to-end, and not modified in-flight. Besides that, I also wonder what happens when folks GPG sign commits that get modified by a workflow, does the attestation get lost in translation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tox -e black-format is only meant to be run manually (not triggered with tox). It is just a shortcut to the specific black command so you do not need to remember it (or install the utility).

tox -e black should run automatically and fail if reformat is needed (we use this approach in other internal repos too).

reviewrot/utils.py Outdated Show resolved Hide resolved
Fixes the following warning:

    reviewrot/gitlabstack.py:227: UserWarning: Calling a `list()` method
    without specifying `get_all=True` or `iterator=True` will return a
    maximum of 20 items.
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