-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Extract default devshell to devenv.nix #42
Conversation
Looks great @sestrella! Very happy to see devenv in the project. Just a few thoughts:
|
@oscar-izval thank you for the provided feedback. Regarding:
I guess the main motivation here is that the existing devshell is primarily used to set up all of the required tools for the update script; in that sense, it is not something that affects the end users of this flake, but rather something we use as maintainers that, in my opinion, should not be exposed in the flake.nix file; I believe this was my main motivation for extracting the devshell from the original file and configuring devenv. Whether we use devenv on its own or as part of a flake integration, I believe it depends on whether there is a valid use case for providing a devshell to the consumers of this flake in the first place. In this case, I would argue that keeping it as a standalone Nix file makes more sense because the provided shell is only used for maintenance.
That was a good call! If you agree with this change, I would be happy to update the documentation. |
After reading your comment looks like the best approach is to keep them separate, thanks for the context! |
7a83554
to
cf3f88e
Compare
@oscar-izval would you mind taking a second look, please? |
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.
LGTM! Maybe just remove those comments, but it's not a big deal. Something to consider is that we were exposing that devshell, and even if nobody uses it, it should technically be a major release. Should we do that alongside the removal of the versions-by-cycle code?
@oscar-izval I merged this PR but canceled the build to avoid generating a major release right now, as I am planning to open a new PR that removes the cycle packages. |
No description provided.