Skip to content

drop lib's self dep and pass data to each function#228

Closed
Pacman99 wants to merge 1 commit intodivnix:corefrom
Pacman99:self-nopass
Closed

drop lib's self dep and pass data to each function#228
Pacman99 wants to merge 1 commit intodivnix:corefrom
Pacman99:self-nopass

Conversation

@Pacman99
Copy link
Member

@Pacman99 Pacman99 commented Apr 3, 2021

Making lib dependent on devos, was not the greatest idea since it prevents others from using the devos lib. And theres quite a few nice lib functions that people might like that they can't use right now: mkPkgs, mkPackages, mkHomeConfigurations.

This gives the specific data to each lib function that that function needs.

@blaggacao
Copy link
Contributor

I think we should rather go the contrary route and only pass deps that are strictly necesary down to second and third order functions.

If a humongous, all-encompassing self needs to be passed through more than one hop, we should rather ask ourselves if it's time for refactoring and untangling the code, before loading it with more functionality.

I think revising/refactoring is in first order, before we can / should implement this feature. Otherwise, we risk in getting this wrong and I don't know the costs of cleanup vs the benefits of the feature.

@Pacman99
Copy link
Member Author

Pacman99 commented Apr 3, 2021

all this PR is doing is reverting an old change. So with or without this, cleanup is necessary. But with this, we can get #218 merged, which makes lib a lot easier to work with, for me at least. And will allow out of tree mkflake usage.
I think the benefits far outweigh the slight harms.

@Pacman99 Pacman99 changed the title drop lib's self dep and pass self to each function drop lib's self dep and pass data to each function Apr 10, 2021
Comment on lines +3 to +4
# main overlay and extra overlays
{ overlay, overlays, extern, overrides }:
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the best way to do this, its hard to combine them all into one argument even though they are related. I chose this method as to not add more implementation logic to combine overlays.

Copy link
Contributor

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

LGTM, as I understand this PR is a cleanup to limit internal api surface! Nice!

@Pacman99 Pacman99 marked this pull request as draft April 10, 2021 15:44
@Pacman99
Copy link
Member Author

Converted to draft, so we only merge once 0.9 is in

pass needed data to each lib function
@Pacman99 Pacman99 marked this pull request as ready for review April 10, 2021 21:56
@Pacman99
Copy link
Member Author

I think it might be good to include this in 0.9(contrary to what I said before), but its not necessary.

@nrdxp
Copy link
Collaborator

nrdxp commented Apr 11, 2021

bors try

bors bot added a commit that referenced this pull request Apr 11, 2021
@bors
Copy link
Contributor

bors bot commented Apr 11, 2021

try

Build succeeded:

@blaggacao
Copy link
Contributor

Superseded by #251

@blaggacao blaggacao closed this Apr 19, 2021
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