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 typed __version__ substitution. #64

Closed
wants to merge 1 commit into from

Conversation

da2ce7
Copy link

@da2ce7 da2ce7 commented Nov 18, 2021

  • Includes basic reformatting of the substitution section in the readme.

* Includes basic reformatting of the `substitution` section in the readme.
@mtkennerly
Copy link
Owner

Hi! Thank you for the PR, and I apologize for the late reply. However, there are a couple of issues here:

  • str is correct, but I don't think it's necessary to be so explicitly restrictive. I'd rather just use something like .*?. Someone could use some slightly different formatting or a type alias, and although it would be unusual, I don't think there's any particular reason to exclude it in the regex.
  • I prefer the original formatting for the README. I think it's more consistent with the rest of the document.

@mtkennerly mtkennerly closed this Jan 21, 2022
@da2ce7
Copy link
Author

da2ce7 commented Jan 21, 2022

Hello @mtkennerly,

You have closed this pull request, would you like me to try and address your comments. Or are you planning to include support for the typed version substitution on your own?

@mtkennerly
Copy link
Owner

I'd be happy to accept the PR if you'd still like to address the comments :) I just wasn't sure how soon you'd see this since I replied late.

@mtkennerly mtkennerly reopened this Jan 21, 2022
@mtkennerly
Copy link
Owner

Closing in favor of b40453b.

@mtkennerly mtkennerly closed this Mar 9, 2022
@da2ce7
Copy link
Author

da2ce7 commented Apr 4, 2022

@mtkennerly thank you, just now I was meaning to get around to addressing the comments and now see you have fixed it already 👍.

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