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

Fix CI breakage #2826

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Fix CI breakage #2826

merged 1 commit into from
Sep 19, 2023

Conversation

fingolfin
Copy link
Member

... caused by two PRs which each was fine by its own but together they broke

@fingolfin
Fix CI breakage
0cc19d4 
... caused by two PRs which each was fine by its own but together they broke
lgoettgens
lgoettgens approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

This looks indeed weird. If CI succeeds, this change is fine with me.

@fingolfin
Copy link
Member Author

Just to explain what's going on: with_unicode() used to (accidentally) return the return value of the final allow_unicode call in it, which always was true. As a result, the doctests introduced in PR #2824 (which invoke with_unicode) printed those true values without me noticing. But in PR #2822 I refactored with_unicode and as an (unintended but IMHO desirable) side effect it now returns nothing. Boom

@fingolfin fingolfin enabled auto-merge (squash) September 19, 2023 21:39
@fingolfin fingolfin enabled auto-merge (squash) September 19, 2023 22:28
@fingolfin fingolfin merged commit 142ea8c into oscar-system:master Sep 19, 2023
10 of 12 checks passed
@lgoettgens lgoettgens mentioned this pull request Sep 20, 2023
Merged
@fingolfin fingolfin deleted the mh/fix-ci branch September 20, 2023 06:39
fieker pushed a commit that referenced this pull request Sep 29, 2023
@fingolfin @fieker
Fix CI breakage (#2826)
1dc10a4 
... caused by two PRs which each was fine by its own but together they broke
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgoettgens lgoettgens lgoettgens approved these changes

@thofma thofma Awaiting requested review from thofma

@ThomasBreuer ThomasBreuer Awaiting requested review from ThomasBreuer

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

Successfully merging this pull request may close these issues.
2 participants
@fingolfin @lgoettgens