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

gh-89886: Use Autoconf quadrigraphs where appropriate #105226

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 2, 2023

@erlend-aasland
Copy link
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.

@corona10
Copy link
Member

corona10 commented Jun 2, 2023

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

@erlend-aasland
Copy link
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.

configure.ac Show resolved Hide resolved
@corona10 corona10 requested a review from vstinner June 2, 2023 09:25
Copy link
Member

@vstinner vstinner left a 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 :-)

@erlend-aasland
Copy link
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.

Copy link
Member

@corona10 corona10 left a 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.

@erlend-aasland
Copy link
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!

@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
Copy link
Contributor

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

@miss-islington
Copy link
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

@erlend-aasland erlend-aasland deleted the autoconf/quadrigraphs branch June 7, 2023 07:11
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jun 7, 2023
@bedevere-bot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants