GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-89886: Use Autoconf quadrigraphs where appropriate by erlend-aasland · Pull Request #105226 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jun 2, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Improve and simplify configure.ac checks #89886

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@erlend-aasland
pythongh-89886: Use Autoconf quadrigraphs to fix syntax highlighting
56ce300
@erlend-aasland
erlend-aasland requested a review from corona10 as a code owner June 2, 2023 08:54
@erlend-aasland erlend-aasland added skip news needs backport to 3.12 only security fixes labels Jun 2, 2023
@bedevere-bot bedevere-bot added the awaiting core review label Jun 2, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 2, 2023
Improve and simplify configure.ac checks #89886
Closed
@erlend-aasland

erlend-aasland commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

@corona10 are you ok with this? In my editor (Vim), this fix is very helpful, not specifically because of the highlighting, but also because it enables Vim to correctly recognise closing parentheses and brackets. This makes hacking Autoconf way easier, since I can quickly verify opening/closing brackets and parentheses using the % char.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10

corona10 commented Jun 2, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Yeah yeah, I understood what you intended. I have the same issue with my vim editor.
What do you think about writing documentation for configure.ac?
Now some semantics is not intuitive to the unfamiliar user. But if the documentation exists, it would be helpful to write new things or maintain the configure.ac

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

What do you think about writing documentation for configure.ac?

I don't think we need to maintain our own Autoconf docs; the GNU Autoconf docs are well written and full of good examples.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10
corona10 reviewed Jun 2, 2023
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
corona10 requested a review from vstinner June 2, 2023 09:25
vstinner
vstinner reviewed Jun 3, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The code correctly works as expected. %:@include is surprising and looks specific to autotools, whereas C uses #include. The benefit is to fix syntax highlight? I'm not convinced that it's worth it.

@corona10 are you ok with this? In my editor (Vim), this fix is very helpful, not specifically because of the highlighting, but also because it enables Vim to correctly recognise closing parentheses and brackets. This makes hacking Autoconf way easier, since I can quickly verify opening/closing brackets and parentheses using the % char.

Well, that was just my opinion. Now it's up to you. You're way more active on changing configure than me :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland and corona10 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@erlend-aasland

erlend-aasland commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

Now it's up to you. You're way more active on changing configure than me :-)

Thanks for you input. I'll abide with Dong-hee's final word on this :)

I won't repeat my arguments as to why I think this is worth it (see #105226 (comment)), but (surprise, surprise) I still would like to land this :) An alternative could be to make sure strings including # are not followed by a closing bracket (quote) on the same line, however such a PR will introduce a larger diff to configure.ac, and it will also introduce changes to the generated configure file; I don't think that alternative is a better choice. This PR was designed to not alter the generated configure file, in order to keep the amount of churn to a minimum. IMO, it is an improvement to status quo.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10
corona10 approved these changes Jun 7, 2023
View reviewed changes

@corona10 corona10 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Okay, let's go.
From my conservative view it is +0.5.. but doing something is always better than doing nothing.
Someone may complain but soon adapt to a new convention.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 7, 2023
@erlend-aasland

erlend-aasland commented Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Someone may complain but soon adapt to a new convention.

As I said in #105226 (comment), there is already precedent for using quadrigraphs in configure.ac (we already do for stuff like brackets) ;)

Thanks, both!

corona10 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland changed the title gh-89886: Use Autoconf quadrigraphs to fix syntax highlighting gh-89886: Use Autoconf quadrigraphs where appropriate Jun 7, 2023
@erlend-aasland
erlend-aasland merged commit 27c68a6 into python:main Jun 7, 2023
@miss-islington

miss-islington commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Jun 7, 2023
@miss-islington

miss-islington commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

Sorry @erlend-aasland, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 27c68a6d8f20090310450862c2c299bb7ba3c160 3.12

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland deleted the autoconf/quadrigraphs branch June 7, 2023 07:11
@miss-islington miss-islington assigned erlend-aasland Jun 7, 2023
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jun 7, 2023
@erlend-aasland
[3.12] pythongh-89886: Use Autoconf quadrigraphs where appropriate (p…
879bb67
…ython#105226)
@bedevere-bot

bedevere-bot commented Jun 7, 2023

Copy link
Copy Markdown

GH-105423 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 7, 2023
erlend-aasland added a commit that referenced this pull request Jun 7, 2023
@erlend-aasland
[3.12] gh-89886: Use Autoconf quadrigraphs where appropriate (#105226) (
c607551
#105423)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@corona10 corona10 corona10 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@erlend-aasland erlend-aasland

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@erlend-aasland @corona10 @miss-islington @bedevere-bot @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.