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

The Big Weeder Cleanup #1157

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

The Big Weeder Cleanup #1157

wants to merge 9 commits into from

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Jun 20, 2024

Description

A big cleanup of unused functions plus moving the resource registry and the normalform vars into separate packages

@jasagredo jasagredo force-pushed the js/weeder branch 4 times, most recently from 4f8aebf to 58a3314 Compare June 21, 2024 13:51
@jasagredo jasagredo changed the title The Big Weeder Decontamination (wip) The Big Weeder Decontamination Jun 21, 2024
@jasagredo
Copy link
Contributor Author

jasagredo commented Jun 21, 2024

This is missing some few things already:

  • HLint: now some pragmas are redundant
  • Changelog entries
  • Add these packages to the badges and to the scripts

@jasagredo
Copy link
Contributor Author

I added the no changelog label because it doesn't make sense to add a changelog fragment for the new packages as they are precisely new

@jasagredo jasagredo marked this pull request as ready for review June 21, 2024 14:11
@jasagredo jasagredo self-assigned this Jun 25, 2024
@jasagredo jasagredo changed the title The Big Weeder Decontamination The Big Weeder Cleanup Jul 25, 2024
@jorisdral
Copy link
Contributor

I don't think nf-vars should be in its own package. If it is preferred to remove it from consensus, then I think the functionality should go into strict-checked-vars

Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! 🧹

@@ -0,0 +1,75 @@
cabal-version: 3.0
name: nf-vars
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, why is it called nf-vars? 🤔 I guess it's due to "normal form". What about strict-vars?

(newTMVar, newTMVarIO, traceTMVar, traceTMVarIO)
import Control.Concurrent.Class.MonadSTM.TBQueue as LazySTM
import Control.Concurrent.Class.MonadSTM.TQueue as LazySTM
import Control.Concurrent.Class.MonadSTM.Strict.SVar hiding
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use this opportunity to import explicitly.

@@ -116,7 +116,7 @@ data instance Block.StorageConfig TestBlock = TestBlockStorageConfig
Mempool support
-------------------------------------------------------------------------------}

newtype instance Ledger.GenTx TestBlock = TestBlockGenTx { unGenTx :: Tx }
newtype instance Ledger.GenTx TestBlock = TestBlockGenTx Tx
Copy link
Member

Choose a reason for hiding this comment

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

It's not unlikely that we might want to use this function at some point, so I don't necessarily agree with weeder's suggestion here, especially considering how little code we're removing.

@@ -174,7 +174,7 @@ instance CanHardFork xs

applyBlockLedgerResult cfg
(HardForkBlock (OneEraBlock block))
(TickedHardForkLedgerState transition st) =
thfls =
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the previous version more readable?

I couldn't see any references for these in cardano-{api,cli,node}, and even if,
they should be easy to replace.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

4 participants