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] commit tags #871

Merged
merged 22 commits into from
Mar 25, 2022
Merged

Conversation

felixdittrich92
Copy link
Contributor

update commit tags from:
https://github.com/mindee/doctr/tags

Copy link
Collaborator

@charlesmindee charlesmindee left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #871 (89ee564) into main (509fce8) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #871      +/-   ##
==========================================
- Coverage   94.84%   94.82%   -0.02%     
==========================================
  Files         133      133              
  Lines        5200     5200              
==========================================
- Hits         4932     4931       -1     
- Misses        268      269       +1     
Flag Coverage Δ
unittests 94.82% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
doctr/transforms/functional/base.py 95.65% <0.00%> (-1.45%) ⬇️
doctr/transforms/modules/base.py 94.59% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 509fce8...89ee564. Read the comment docs.

@charlesmindee charlesmindee merged commit 7f396ca into mindee:main Mar 25, 2022
@felixdittrich92 felixdittrich92 deleted the fix-commit-tags branch April 5, 2022 14:20
felixdittrich92 added a commit to felixdittrich92/doctr that referenced this pull request Apr 5, 2022
* backup

* replace tags with actual
felixdittrich92 added a commit to felixdittrich92/doctr that referenced this pull request Apr 7, 2022
* backup

* replace tags with actual
@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

Hey @felixdittrich92 :)
Quick question: is this really a fix? Or a refactor? Because I don't remember that there are constraints on the length of the hash prefix to specify it 🤔

@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

@charlesmindee for the next release note, I suggest adding the proper tags here :)
Perhaps "type: enhancement" & "ext: docs"

@felixdittrich92
Copy link
Contributor Author

@frgfm 👋 ,
I am not sure after merging the postrelease modifications all the tags have changed which was really confusing 😅 I can not really explain why this happens maybe in fact of removing some sphinx stuff !? So i would say in this case it was more a fix

@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

But what do you mean by "the tag changed" ? :) This PR is simply removing the last letter of every single tag, and I'm trying to understand why this was needed 🙃

@felixdittrich92
Copy link
Contributor Author

felixdittrich92 commented Apr 27, 2022

Yes all tags (there ids from the last PR) in https://github.com/mindee/doctr/tags have changed after the documentation update / or the postrelease merge ( i am not sure i have faiced this issue by checking the documentation after the postrelease)

@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

I mean, the tags haven't changed :)
Take the first tag for instance https://github.com/mindee/doctr/releases/tag/v0.5.1, you see "9d03085" but this is only the first character of the full hash (cf. 9d03085)

So here, whether you use the first 7 or 8 characters, git is flexible enough to accept it. Initially we had 8 chars, and it has been working up until now :)

@felixdittrich92
Copy link
Contributor Author

I think i got it any last character was set wrong (from my side) so the documentation has not build correctly -> i have used only the visible hash to fix this from tags and it worked 😅 ok than it was a mistake from me

@felixdittrich92
Copy link
Contributor Author

But i think only to take the visible hash is also save and will avoid some mistakes again wdyt ?

@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

But i think only to take the visible hash is also save and will avoid some mistakes again wdyt ?

Sure, but if you click on 9d03085, you can see in the URL the full hash. The number of chars is not exactly relevant (i think github requires a minimum, most likely 7)

@felixdittrich92
Copy link
Contributor Author

Now after clarification i would say it was a refactor and not a fix or better a stupid mistake 😓 😅

@frgfm
Copy link
Collaborator

frgfm commented Apr 27, 2022

Not a mistake don't worry, I was just trying to understand whether github updated the max number of chars for hash resolution or something :)

You can rename the PR "fix" --> "refactor" to better help for PR sorting/labelling for the next release though 👍

@felixdittrich92 felixdittrich92 changed the title [Fix] commit tags [Refactor] commit tags Apr 27, 2022
@frgfm frgfm added type: misc Miscellaneous ext: docs Related to docs folder labels May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: docs Related to docs folder type: misc Miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants