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

Rename temp::Cfg to temp::Context #3761

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Rename temp::Cfg to temp::Context #3761

merged 2 commits into from
Apr 9, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Apr 9, 2024

While reviewing #3754 I was a little surprised to note that the temp::Cfg type was different from config::Config (and then apparently there's also dist::config::Config). As far as I can see temp::Cfg also really doesn't represent "configuration" in the sense that it's input provided by the user or an external process. Renamed it to TemporaryContext instead.

@djc djc requested a review from rami3l April 9, 2024 08:24
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

The intention of the original naming is that it should be referenced with temp::Cfg, although I think Cfg is definitely confusing...

Frankly speaking I would personally prefer temp::Context, but since we already have other stuff like download::DownloadCfg I think this is acceptable as well.

@rami3l
Copy link
Member

rami3l commented Apr 9, 2024

BTW pedantically speaking we should have a BREAKING marker on the commit message that changes the API name :)

@djc
Copy link
Contributor Author

djc commented Apr 9, 2024

Frankly speaking I would personally prefer temp::Context, but since we already have other stuff like download::DownloadCfg I think this is acceptable as well.

That's fair -- changed it to temp::Context instead.

BTW pedantically speaking we should have a BREAKING marker on the commit message that changes the API name :)

Why? Is there anything tracking those? IIRC we don't even pretend to have a stable library API. (FWIW in the many projects that I maintain none use BREAKING markers in commit messages.)

@rami3l
Copy link
Member

rami3l commented Apr 9, 2024

BTW pedantically speaking we should have a BREAKING marker on the commit message that changes the API name :)

Why? Is there anything tracking those? IIRC we don't even pretend to have a stable library API. (FWIW in the many projects that I maintain none use BREAKING markers in commit messages.)

@djc I agree with you that we don't have a stable set of external APIs. OTOH I do have the impression somewhere down the issue threads there's a post saying in the long run we're moving this way... but I failed to find the exact location. Anyway, I think it'd be better to just forget about what I said on this one for now 🤦‍♀️

@rami3l rami3l changed the title Rename temp::Cfg to TemporaryContext Rename temp::Cfg to temp::Context Apr 9, 2024
@djc djc enabled auto-merge April 9, 2024 09:23
@djc djc added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2024
@rami3l rami3l added this pull request to the merge queue Apr 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 9, 2024
@rami3l
Copy link
Member

rami3l commented Apr 9, 2024

@djc There seems to be a conflict with #3763 that went undetected by GitHub but has been caught by the CI. (Thanks, GHMQ!)

@rami3l rami3l enabled auto-merge April 9, 2024 10:00
@rami3l rami3l added this pull request to the merge queue Apr 9, 2024
Merged via the queue into master with commit 1754da3 Apr 9, 2024
21 checks passed
@rami3l rami3l deleted the temp-tweaks branch April 9, 2024 11:08
@rbtcollins
Copy link
Contributor

 @djc I agree with you that we don't have a stable set of external APIs. OTOH I do have the impression somewhere down the issue threads there's a post saying in the long run we're moving this way... but I failed to find the exact location. Anyway, I think it'd be better to just forget about what I said on this one for now 🤦‍♀️

We don't have an external API at all.

I think we should have a process interface that is machine readable - e.g. protobuf - but not a use-as-library interface, since we have little to no evidence that there is a need for a library interface.

So there is no need to pay a compatibilty tax on APIs

@rami3l rami3l added this to the 1.27.1 milestone Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants