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.

Sign up for GitHub

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

Jump to bottom

add test for except star #237

Merged
merged 14 commits into from
Nov 2, 2022
Merged

add test for except star #237

merged 14 commits into from
Nov 2, 2022

Conversation

eikichi18
Copy link
Contributor

Add test for except star

loechel
loechel requested changes Oct 15, 2022
View reviewed changes
Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

see specific comments for changes

.gitignore Outdated Show resolved Hide resolved
tests/transformer/test_try.py Outdated Show resolved Hide resolved
tests/transformer/test_try.py Outdated Show resolved Hide resolved
loechel
loechel approved these changes Oct 15, 2022
View reviewed changes
Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

Now looks good for me.

@icemac please make final decission on merge

ale-rt
ale-rt approved these changes Oct 15, 2022
View reviewed changes
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

LGTM, requires PR #238 to be merged first

icemac
icemac requested changes Oct 26, 2022
View reviewed changes
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

I am not sure whether we actually need to support this new esoteric syntax: I've never had a need for it over all the years. But as it seems easy to support it, let's keep it.

The only additional change request I have is a change log entry.

tests/transformer/test_try.py Outdated Show resolved Hide resolved
@eikichi18 eikichi18 requested a review from icemac October 26, 2022 15:35
@icemac
Copy link
Member

icemac commented Oct 27, 2022

The code looks good now. The change log entry is still requested. Maybe we have to lower the required code coverage, so the tests can pass. (Instead of waiting for #238.)

loechel
loechel requested changes Oct 28, 2022
View reviewed changes
Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

@eikichi18 can you please also add a changelog entry in CHANGES.rst,

@eikichi18
Copy link
Contributor Author

Done

@dataflake dataflake merged commit 688bec4 into zopefoundation:master Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perrinjerome perrinjerome perrinjerome left review comments

@loechel loechel loechel approved these changes

@ale-rt ale-rt ale-rt approved these changes

@icemac icemac Awaiting requested review from icemac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eikichi18 @icemac @perrinjerome @ale-rt @loechel @dataflake