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

Remove direct imports from Cardano.Ledger #558

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

felix-lipski
Copy link
Contributor

@felix-lipski felix-lipski commented Jan 8, 2024

Changelog

- description: |
    Update [cardano-api-8.39.0.0](https://github.com/IntersectMBO/cardano-api/blob/main/cardano-api/CHANGELOG.md#83900).
    Removed direct imports from `Cardano.Ledger`.
# uncomment types applicable to the change:
  type:
  - improvement    # refactoring

Context

Issue

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@felix-lipski felix-lipski changed the title remove cardano ledger imports Removing direct imports from Cardano.Ledger Jan 8, 2024
@felix-lipski felix-lipski changed the title Removing direct imports from Cardano.Ledger Remove direct imports from Cardano.Ledger Jan 8, 2024
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Looks good however you should ask the team if they prefer Ledger or L as the qualified import. We should pick one and stick to it.

cardano-cli/cardano-cli.cabal Outdated Show resolved Hide resolved
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Can we switch to using one qualified import alias, either L or Ledger?
I'd pick L, seems pretty obvious to me.

@palas palas self-assigned this Feb 12, 2024
@palas palas force-pushed the felix-lipski/remove-cardano-ledger-imports branch 2 times, most recently from 22b2463 to 47eaafa Compare February 13, 2024 02:48
@palas
Copy link
Contributor

palas commented Feb 13, 2024

Can we switch to using one qualified import alias, either L or Ledger?
I'd pick L, seems pretty obvious to me.

I've changed it everywhere in: 47eaafa

Is that something we want to do?

@palas palas force-pushed the felix-lipski/remove-cardano-ledger-imports branch from b2bdfe1 to 9c4ea75 Compare February 13, 2024 13:20
@smelc
Copy link
Contributor

smelc commented Feb 13, 2024

In favor of L over Ledger 👍

@palas palas marked this pull request as ready for review February 13, 2024 16:39
cabal.project Outdated
Comment on lines 25 to 30
source-repository-package
type: git
location: https://github.com/IntersectMBO/cardano-api.git
tag: f59090668707b8960a057435a507924ae18be16e
subdir: cardano-api
--sha256: sha256-TJeLlUmZhlckBqQNQVSyFySM66bWa9ggIIOtnNBi0Hs=
Copy link
Contributor

Choose a reason for hiding this comment

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

Since - generally speaking - we don't allow SRPs on main, you'll need to wait for cardano-api to be released and updated to merge.

Copy link
Contributor

@smelc smelc left a comment

Choose a reason for hiding this comment

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

A net improvement, thank you 🙏

I'm approving, but you'll need to wait for a release and update of cardano-api to merge.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM. You can squash the commits. Also we can't merge yet until we have another release of cardano-api.

@palas palas force-pushed the felix-lipski/remove-cardano-ledger-imports branch 3 times, most recently from e29b800 to d77d91c Compare February 23, 2024 04:40
@palas palas enabled auto-merge February 23, 2024 05:13
@palas palas force-pushed the felix-lipski/remove-cardano-ledger-imports branch from d77d91c to ac64002 Compare February 23, 2024 05:26
@palas palas added this pull request to the merge queue Feb 23, 2024
Merged via the queue into main with commit 7fad582 Feb 23, 2024
16 checks passed
@palas palas deleted the felix-lipski/remove-cardano-ledger-imports branch February 23, 2024 05:50
@palas palas mentioned this pull request Feb 23, 2024
3 tasks
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.

5 participants