Cnf over conf#11176
Conversation
|
It's a bit interesting to see how we struggle with these sort of things. After unabbreviating lcl -> locl -> local, we now go the other way in another part. In my view, cnf vs conf is like htm vs html. |
|
There are conflicts, please rebase. |
|
pesky makdown :) rebased and pushed. |
The default is openssl.cnf The project seems to prefer xxx.conf these days, but we should use the default convention. Rename all foo.conf (except for Configurations) to foo.cnf Fixes #11174
InfoHunter
left a comment
InfoHunter
left a comment
There was a problem hiding this comment.
It seems there is a consensus made in #11174 , so I think this could be approved.
t8m
added
approval: done
FWIW: If it weren't for |
|
Don't know why the "ready to merge" robot didn't catch this, @iamamoose but this seems like 24 hours are done. |
IIUC, the bot refrains from setting the "ready to merge" state if someone posts additional comments after the approval. In this case it was probably my post which prevent it. The rationale behind it is that a comment after final approval might be an indication of an ongoing discussion. |
t8m
added
approval: ready to merge
Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #11176)
The default is openssl.cnf The project seems to prefer xxx.conf these days, but we should use the default convention. Rename all foo.conf (except for Configurations) to foo.cnf Fixes #11174 Reviewed-by: Paul Yang <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #11176)
|
Pushed to master. |
|
Thanks for the discusion and review! |
Exactly that. I don't want it to start second guessing or being too clever -- if there's any change to a PR since the Done label was set (such as a comment), then it assumes it needs a human decision. |
|
"resolved - working as designed" :) makes sense to me, just thought it was worth asking. |
|
But OTOH we loose the comfort of being reminded. Let's assume @iamamoose that you change the bot to neglect commits after 'review done', then we still would have two options to prevent the automatism:
Maybe that alternative would be more convenient? |
|
Another possibility is to just change the message to "This appears to be ready to merge (the hold period has expired)" |
|
I like that suggestion, but I would drop the part in the parentheses:
@iamamoose what do you think about ignoring additional posts and triggering the timeout unconditionally, with a slightly modified (weaker) message? |
|
I'm not a fan of labels that can mean two things; it could mean "this is ready do merge" or "this is probably ready to merge once you've read all the recent changes and they're harmless". Two possibilities for "if there were changes made since the approval:done flag was set":
I went ahead and did #2 for now. It also does similar when CI is failed. |
6 participants
I know foo.conf is what we seem to prefer. But foo.cnf is what the docs talk about, and openssl.cnf is the default. I think it's less confusing to use .cnf as the extension for OpenSSL config files, so do the big renaming.