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

feat(python): add poetry support #188

Merged

Conversation

jgrant216
Copy link

Resubmitted/adjusted from conventional-changelog#643

@jgrant216
Copy link
Author

@TimothyJones, can you take a look at this when you have time?

Copy link
Member

@TimothyJones TimothyJones 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 reminder! LGTM, and I've put a couple suggestions inline.

I'll run the build, but it will fail without the autoformatter being run on this.

I'm heading out the door, but if the formatter hasn't been run by the next time I look at this, I'll push a commit that does it.

@@ -0,0 +1,29 @@
const semverRegex = /version[" ]*=[ ]*["'](.*)["']/i
Copy link
Member

Choose a reason for hiding this comment

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

This variable is a bit misleadingly named - it might be better as versionExtractRegex or similar, since it's not really testing for semver.

if (found == null) {
return false
}
version = found[1]
Copy link
Member

Choose a reason for hiding this comment

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

Optional: I had to read this a couple times to see why version wasn't always undefined. I reckon it'd be clearer if it got the index, and then extracted the version in separate steps. This is because people making changes to open source are often in a hurry, so it's worth optimising for ability to skim-read accurately.

Copy link
Author

Choose a reason for hiding this comment

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

@TimothyJones , thanks for the feedback. I updated the variable name from found to versionMatcher and added a comment as to returning false when no version is found. Please let me know if you want this adjusted further.

@TimothyJones
Copy link
Member

Oh, and could you add a note to the readme that explains how to use it, and any caveats for Python people?

@jgrant216
Copy link
Author

@TimothyJones , reminder to follow up on this.

@TimothyJones
Copy link
Member

Oh, sorry - I don't get notified unless there's new comments. I'll take a look now

@TimothyJones
Copy link
Member

Needs npm run format:fix to be run - I'll check it out and push a commit.

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.80%. Comparing base (7ebdacd) to head (4f7b4a5).
Report is 26 commits behind head on master.

Files with missing lines Patch % Lines
lib/updaters/index.js 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   94.11%   92.80%   -1.32%     
==========================================
  Files          26       29       +3     
  Lines         493      542      +49     
==========================================
+ Hits          464      503      +39     
- Misses         29       39      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TimothyJones TimothyJones merged commit 01f08e9 into absolute-version:master Oct 10, 2024
8 checks passed
@TimothyJones
Copy link
Member

Releasing now. I'll also figure out why I didn't get notified about your reply in the comment thread. Sorry about that!

Thanks again for the PR!

@TimothyJones
Copy link
Member

Released as 12.5.0

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