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

Refactor buildComponent and replComponent #9475

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Nov 24, 2023

This is a pure refactor that reduces code duplication, with the exception that it also fixes a bug in that preprocessExtras was not being called for foreign libraries in both functions.

@sheaf sheaf marked this pull request as draft November 24, 2023 13:50
@sheaf sheaf force-pushed the wip/buildComponent branch 2 times, most recently from 08db0f1 to 488f7c2 Compare November 27, 2023 10:43
@alt-romes alt-romes force-pushed the wip/buildComponent branch 4 times, most recently from 01991b1 to 9db2722 Compare November 27, 2023 11:52
@sheaf sheaf marked this pull request as ready for review November 29, 2023 10:02
Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

This looks like an improvement thanks! TBH I still find this code a bit hard to read :-/
I particularly do not like those invalid cases arising with CTest/CBench.

@andreabedini
Copy link
Collaborator

While I am ok with this going in as it is (as said, it is an improvement!) I am quite unhappy with the general pattern we seem to have through the codebase of pushing case-splits "down" the data flow. IMHO they should be pushed up instead.

This is a pure refactor that reduces code duplication, with the
exception that it also fixes a bug in that `preprocessExtras` was
not being called for foreign libraries in both functions.
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 12, 2023

Nobody commented in 2 weeks, so let me set the label for @sheaf not to delay the merge even more.

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label Dec 12, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 14, 2023
@mergify mergify bot merged commit 4c1d382 into haskell:master Dec 14, 2023
48 checks passed
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
This is a pure refactor that reduces code duplication, with the
exception that it also fixes a bug in that `preprocessExtras` was
not being called for foreign libraries in both functions.

Co-authored-by: Rodrigo Mesquita <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants