-
Notifications
You must be signed in to change notification settings - Fork 21
Centralize deprecations with date #114
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
base: main
Are you sure you want to change the base?
Conversation
9c35b50
to
9d26c4a
Compare
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.
Thanks! Needs some changes.
b93299c
to
dda55d0
Compare
dda55d0
to
37e3aa9
Compare
Additionally rebased and added the removed |
(mkRemovedOptionModule | ||
["rum" "programs" "git" "destination"] | ||
"The default destination is now under `~/.config/git`") |
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.
Should we have a removal date for these too?
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.
Not really, as this would be part of the history. Once it's removed it's removed, I don't really see a particular reason why we should add a date to this. I guess it might be a little clearer from the user's POV?
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.
Well, you could say the same for a renamed module. Is the point not that including the mkRemovedOption increases eval time and messyness of the codebase in the longterm, just like mkRenamedOption ?
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.
Agreed, this shouldn't get special treatment.
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.
Oh, you meant it like that. We could just add a comment for removed options, as they are well, already removed (the users don't have to know but we do)
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.
Could you clarify what you mean?
I think what Lunar and I are saying is that we should still use mkRemovedOption
, but include a timer for removal, just like mkRenamedOption
. I'm fine with keeping a comment around after removal, if that's what you meant.
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.
I meant that instead of having our wrapper function that gives the consumer of our module a warning with the upcoming removal date, it should just be a comment on top of the MkRemovedOptionModule
call, because the option will just be removed so there's no need to warn the user that the mkRemovedOptionModule
will eventually be removed.
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.
My worry with comments is that they're harder to remember to delete - I get what you mean, though.
Speaking of remembering to delete things, do we have any plans for automation around deleting stuff after the expiration date?
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.
That would be great. The only issue is that this would likely require a custom Python script using tree sitter, or rnix-parser, which doesn't seem to have been updated in a little while and does not even build (the flake seems to have Cargo related errors).
If anybody would like to dive into writing a custom script for the pipeline, that would be amazing, but it is non blocking for now.
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.
Or just a regex string, if you're crazy like I am...
Super busy with other stuff right now, but I'll try to get back to this when I have the chance.
37e3aa9
to
47a12b3
Compare
Tracking: NixOS/nixpkgs#427073 |
fd81a5f
to
7a3875a
Compare
7a3875a
to
819b8af
Compare
819b8af
to
2bd397f
Compare
Title. This centralizes every deprecation in a single file, making it a lot easier for developers/consumers (we should also add a changelog, but this is for another PR). I also added an arbitrary baseline deprecation window of 3 months, but this can be discussed here (I'm open to suggestions).
Meta
Related Issue(s): <None>
AI used to generate code included in this PR?: No
All Submissions:
New Module Submissions: