Skip to content

Refactors (ApplicativeDo, record syntax, fetchWith) #544

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

Merged
merged 7 commits into from
Mar 7, 2022

Conversation

Profpatsch
Copy link
Member

These are a few code refactors,

  • use ApplicativeDo where possible
  • use explicity record syntax instead of ordering-dependent constructors
  • reduce complexity of fetchWith by moving most arguments into a single enum in multiple steps

Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

This PR is a delight. Thank you very much!

Just have two tiny nitpicks.

I’ll give others a chance to chime in as well. But from my point of view this is basically good to go.

Just out of curiosity: What was your motivation for this PR?

@sternenseemann
Copy link
Member

(force-pushed to trigger CI with 8.8.4)

@Profpatsch
Copy link
Member Author

Just out of curiosity: What was your motivation for this PR?

I … accidentally looked at the code and had a bunch of time 😅

@Profpatsch Profpatsch force-pushed the fetchWith-refactors branch from 637eddc to 8fa2df7 Compare March 7, 2022 09:55
This makes referencing fields a lot easier, and has been possible
since GHC 8.0.1.
Improves readability quite considerably.
The reading flow is just a lot better if case matches are used.
We have a complete list of sources, thus we can use a simple enum to
list all sources, this also leads to less ad-hoc string concatenation.

The boolean logic should stay the same, but the double negation with
`fetched` had me thinking for a bit.
From the DerivKind we can determine the command directly.

This is still slightly not-nice, since the extra args depend on the
prefetch command, but we might be able to remove that indirection as
well.
Instead, add the few extra args that we want to pass to the
`DerivKind` enum, which is

* Whether to fetch git submodules
* Whether to unpack an archive from a url/zip source
@Profpatsch Profpatsch force-pushed the fetchWith-refactors branch from 8fa2df7 to 17d4704 Compare March 7, 2022 09:59
@Profpatsch
Copy link
Member Author

I had some weird rebasing issues, but now it should be rebased on master & the comments fixed

@maralorn
Copy link
Member

maralorn commented Mar 7, 2022

Okay, thanks!

@maralorn maralorn merged commit 880c049 into NixOS:master Mar 7, 2022
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