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

Fix, simplify, and improve certain aspects of the project. #24

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

Conversation

wyattscarpenter
Copy link

Recently, I began to use Publish for myself, as it sort of matches my workflow. While trying it out, I encountered several errors and edge cases, which I have now fixed. Here is an abridged listing of my changes:

  • Don't do the tag.tag dance. The tag is always the tag. This might break compatibility with some other system, but it fixes compatibility on mine.
  • Assert the existence of GITHUB_TOKEN upfront, because without it we just crash later.
  • Simplify some code.
  • Enforce semver prohibition on leading zeros in regex.
  • Bump the required version of Twine to avoid a KeyError.
  • Force now only forces through dirty, not tags; thus matching its help message.
  • Print out a couple more diagnostics, and use stderr for more of them, under an internal logic I cannot explicitly justify, but which seems sort of right.
  • General reformatting.

@wyattscarpenter wyattscarpenter requested a review from a team as a code owner September 2, 2024 08:29
Copy link

@perj perj 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 PR and sorry about the slow reply time.

I think this is a good change overall, catching problems earlier is good and I'm all for switching to f-strings.

I only really have one comment on the code, as you can see. But Codady has plenty, that should be solved.

I also think the new exit codes should be described in the README, under usage.

Comment on lines +69 to +78
current_name = self.repo.git.describe(all=True)
print(f"Current name: {current_name}", file=sys.stderr)
current_tag = self.repo.rev_parse(current_name)
print(f"Current tag id: {current_tag}", file=sys.stderr)
try:
return str(tag.tag)
except AttributeError:
return str(tag)
self.current_tag = current_tag.tag
print(f"Current tag: {self.current_tag}", file=sys.stderr)
except (AttributeError, BadName, GitCommandError):
print("Not an annotated tag!", file=sys.stderr)
exit(4)
Copy link

Choose a reason for hiding this comment

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

Since GitCommandError is caught here, I think you might've meant to include the git calls inside the try block.

@wyattscarpenter
Copy link
Author

Glad to hear it! I'll try to fix all these things soon :)

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.

2 participants