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

Support 't' specifier in keywords #1015

Merged
merged 10 commits into from Oct 1, 2023
Merged

Support 't' specifier in keywords #1015

merged 10 commits into from Oct 1, 2023

Conversation

jeanas
Copy link
Contributor

@jeanas jeanas commented Jun 16, 2023

Fixes #1014

@jeanas
Copy link
Contributor Author

jeanas commented Jun 16, 2023

Note that there is a test for parse_keywords but no test for the actual extraction as I didn't really understand how that was supposed to be tested. There seems to be a mixture of pytest and unittest, setuptools integration and not. Which should I use? Or should I just write a test that exercises the inner function directly? Help is welcome.

@jeanas
Copy link
Contributor Author

jeanas commented Jun 16, 2023

CI fails with a seemingly unrelated error.

@akx
Copy link
Member

akx commented Jun 27, 2023

There seems to be a mixture of pytest and unittest, setuptools integration and not.

For new tests, prefer plain pytest style. Don't worry about the setuptools integration, it's... a bit of a mess.

Or should I just write a test that exercises the inner function directly? Help is welcome.

You can write a test that uses exercises the extraction function with a bespoke keywords directly, that's fine!

@jeanas
Copy link
Contributor Author

jeanas commented Jul 1, 2023

(Rebasing on master)

@jeanas
Copy link
Contributor Author

jeanas commented Jul 1, 2023

Thank you. I've added a test and fixed the bug that it caught.

tox -e py311 passes for me locally. I'm not sure what to do about the CI.

@jeanas
Copy link
Contributor Author

jeanas commented Jul 12, 2023

Anything else needed?

@akx akx self-requested a review July 27, 2023 11:24
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

A couple of comments – sorry for the delay...

babel/messages/frontend.py Show resolved Hide resolved
babel/messages/frontend.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1015 (be33f02) into master (9ef53c6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   90.91%   90.94%   +0.03%     
==========================================
  Files          25       25              
  Lines        4358     4375      +17     
==========================================
+ Hits         3962     3979      +17     
  Misses        396      396              
Files Changed Coverage Δ
babel/messages/extract.py 95.31% <100.00%> (+0.12%) ⬆️
babel/messages/frontend.py 87.61% <100.00%> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jeanas
Copy link
Contributor Author

jeanas commented Aug 1, 2023

Thank you for your review, @akx. The PR should be improved quite a bit now, especially with regard to documentation and code clarity.

@jeanas
Copy link
Contributor Author

jeanas commented Sep 1, 2023

@akx Gentle ping?

@akx
Copy link
Member

akx commented Sep 4, 2023

@jeanas Sorry, I was on holiday and all that 😊

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

A couple requests remaining. Thanks for your patience 😅

babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/frontend.py Outdated Show resolved Hide resolved
babel/messages/frontend.py Outdated Show resolved Hide resolved
@jeanas
Copy link
Contributor Author

jeanas commented Sep 6, 2023

A couple requests remaining. Thanks for your patience 😅

Now addressed.

@jeanas
Copy link
Contributor Author

jeanas commented Sep 12, 2023

Ping?

@jeanas
Copy link
Contributor Author

jeanas commented Sep 30, 2023

@akx Gentle ping.

@akx akx self-requested a review October 1, 2023 09:52
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Thank you, and sorry for the long review delays!

@akx akx merged commit 6fc07a2 into python-babel:master Oct 1, 2023
22 checks passed
@jeanas jeanas deleted the t branch October 1, 2023 09:55
@jeanas
Copy link
Contributor Author

jeanas commented Oct 1, 2023

Thank you!

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.

Feature request: Support gettext-like "t" specifier in keywords
2 participants