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

Refactor Category to be Labels #486

Merged
merged 5 commits into from
Jul 11, 2022
Merged

Refactor Category to be Labels #486

merged 5 commits into from
Jul 11, 2022

Conversation

DanielNoord
Copy link
Contributor

Refactor as requested by @ezio-melotti in #485 (comment).

Also couldn't help but notice the unused import so I removed it in a separate commit. If that's undesired I'll revert that change obviously!

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #486 (4ea7afb) into main (6aebdb4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #486   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1815      1815           
  Branches       220       220           
=========================================
  Hits          1815      1815           
Flag Coverage Δ
Python_3.10 100.00% <100.00%> (ø)
Python_3.11-dev 100.00% <100.00%> (ø)
Python_3.8 100.00% <100.00%> (ø)
Python_3.9 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bedevere/prtype.py 100.00% <100.00%> (ø)
tests/test_filepaths.py 100.00% <100.00%> (ø)
tests/test_prtype.py 100.00% <100.00%> (ø)

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

I left a few minor comments but otherwise looks good to me.

However I'm a bit confused:

  • the module docstring says "Label a pull request based on its type"
  • the enum docstring talks about labels for PRs

but:

  • classify_by_filepaths below calls util.issue_for_PR to retrieve the issue
  • add_label/add_category accept an issue and label it

It would appear that is the issue being labeled, and if that's indeed the case, then the module docstring and the enum docstrings should be updated. (I think it's fine doing it in this PR.)

bedevere/prtype.py Outdated Show resolved Hide resolved
bedevere/prtype.py Outdated Show resolved Hide resolved
bedevere/prtype.py Show resolved Hide resolved
@DanielNoord
Copy link
Contributor Author

  • the module docstring says "Label a pull request based on its type"
  • the enum docstring talks about labels for PRs

but:

  • classify_by_filepaths below calls util.issue_for_PR to retrieve the issue
  • add_label/add_category accept an issue and label it

I'm not sure about the gh.post method, but as I see it gh gets labeled (which is the PR) and the issue is being used to retrieve some data about the PR and its related issue.

So I think the current docstrings are actually correct. But somebody with more knowledge about the gh.post method can probably confirm this 100%.

@ezio-melotti ezio-melotti merged commit 83a2d22 into python:main Jul 11, 2022
@DanielNoord DanielNoord deleted the labels branch July 11, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants