Improve documentation & other kinks of marker keyword expression PR #12500

[lovetheguitar]

Follow up to #12500

[webknjaz]

I requested a couple of reviews from those who'd be interested. The RST changes seem fine, I didn't look into the rest, though, so I'm not marking as approved and will let others do this.

One thing to note, though, is that it's usually better to make many small PRs that are atomic over a bit one with a mix of changes. Smaller PRs are faster to merge, and unrelated things don't block parts that could be merged right away. Making atomic PRs would also help you come up with better PR titles and explanations, making them accurate and useful.

[The-Compiler]

One thing to note, though, is that it's usually better to make many small PRs that are atomic over a bit one with a mix of changes. Smaller PRs are faster to merge, and unrelated things don't block parts that could be merged right away. Making atomic PRs would also help you come up with better PR titles and explanations, making them accurate and useful.

...huh? That makes no sense at all. There are 12 changed lines in this PR, and they all are part of what should have been the original review in #12500 in the first place. There's no point in generating that amount of additional noise for addressing reviews of a single PR.

[bluetech]

Thanks!

[bluetech]

Suggested change

	value: (unescaped) str | int | bool | None
	value: (unescaped) string literal | (-)?[0-9]+ | 'False' | 'True' | 'None'

(Even though isdigit is not exactly [0-9] I think we shouldn't promise beyond that)

[lovetheguitar]

Fixed up in 540ede3 now.

[RonnyPfannschmidt]

i believe this one should skip news

[RonnyPfannschmidt]

i wonder if

Suggested change

	not_expr: 'not' not_expr | '(' expr ')' | ident ('(' name '=' value ( ', ' name '=' value )* ')')?
	ident: (\w|:|\+|-|\.|\[|\]|\\|/)+
	name: a valid ident, but not a reserved keyword
	not_expr: 'not' not_expr | '(' expr ')' | ident kwargs?
	ident: (\w|:|\+|-|\.|\[|\]|\\|/)+
	kwargs: ('(' name '=' value ( ', ' name '=' value )* ')')
	name: a valid ident, but not a reserved keyword

helps with readability

[lovetheguitar]

Done in 36b384a.

[webknjaz]

@lovetheguitar pro tip: I recommend accepting suggested changes in PRs using GitHub UI and then pulling them to your computer. The reason is that this would mark the suggester as a co-author, and it's a good tone to credit things that collaborators proposed to their accounts.

Refs:

• https://x.com/hynek/status/1560196397885956097
• https://hynek.me/til/easier-crediting-contributors-github/

[The-Compiler]

Thanks for the follow-up, much appreciated!

[webknjaz]

...huh? That makes no sense at all. There are 12 changed lines in this PR, and they all are part of what should have been the original review in #12500 in the first place. There's no point in generating that amount of additional noise for addressing reviews of a single PR.

While I understand the sentiment, I disagree in general: it might make sense for some parts if they could be seen as related, as they could be merged faster. In general, the more dedicated logical things you bundle, the more time it takes to get them accepted. And some parts might block changes that could be merged right away otherwise. That doesn't unconditionally relate to whether it should've been a part of another PR, as that PR could also bundle several things. So there is a point for me and perhaps for some other people too.

i believe this one should skip news

@RonnyPfannschmidt @lovetheguitar that depends on whether it's important to link this patch.

Pro tip: it is possible to link pre-existing change note to a follow-up PR, in case it's desired/important. Here's how:

$ cd changelog/
$ ln -s {12281,12523}.feature.rst
$ ln -s {12281,12500}.feature.rst
$ git add {12500,12523}.feature.rst
$ git commit
$ git push

When Towncrier sees the same content in several change notes of the same category, it renders it once but links several issues/PRs. It's pretty handy when you have a change that happened to span over a few iterations of polishing/follow-ups.

[nicoddemus]

While I understand the sentiment, I disagree in general

I believe we are all actually in agreement: in general we also prefer separate PRs, but this PR specifically, which addresses small adjustments from a previous PR (mostly one liners), a single PR is preferable. I think that's the point @The-Compiler was trying to make.

[webknjaz]

@nicoddemus fair, but my point still applies — this has gone through like 2-3 rounds of reviews. And there were parts where no changes were requested, just sitting and waiting. I'd even merge those uncontroversial ones myself if they were separate.
Note that I explained above that multiple PRs can be linked to a change note. So if this is important to present to the end-users, we'd still be able to link them all.
Of course, it's fine as it is now. However since Christian was asking about improving the way he contributes to open source earlier, I felt like it's important to communicate this.