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

Deprecate some patterns, remove their internal use #728

Merged
merged 5 commits into from
Jan 20, 2025

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Jan 16, 2025

Changelog

- description: |
    Deprecate some patterns, remove deprecated ones. See the deprecation stanzas for migration guidance.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Our codebase is hard to enter. It's big, it redefines types with the same names from the ledger, it has a complex witness system, it has historical quirks. This PR aims at reducing one of the things you need to understand to contribute to this codebase: patterns. The patterns that are deprecated by this PR are easily replaced by a few functions calls there and there. This shows on the count of lines changed: it's more deletion (51) than additions (22).

In additions, this barely impacts the CLI, as the corresponding CLI PR shows.

For the record, this PR is a spin-off of this work: #724

Follow-up

If this one obtains support, I can go further and deprecate more patterns. Up to reviewers to tell me which direction to go. [edit] Approval obtained, see: #728 (review)

I will also create an issue to follow-up on definitely removing the patterns that are deprecated by this PR.

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
  • Self-reviewed the diff

@smelc smelc force-pushed the smelc/remove-patterns branch from a3e7827 to 74c7f2e Compare January 16, 2025 13:08
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.

I don't know about this change - I don't know how popular are patterns across cardano-api consumers. Those patterns are convenient, but somewhat magical, like TxBody, doing suddenly a whole conversion.

@Jimbo4350 thoughts?

Comment on lines -55 to -56
, AsByronTx
, AsShelleyTx
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

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 however I would not delete the patterns off the bat. Deprecate them first and after 2 major releases we can remove them.

@smelc smelc force-pushed the smelc/remove-patterns branch from 74c7f2e to 0e25624 Compare January 20, 2025 09:24
@smelc smelc changed the title Reduce usage of patterns Deprecate some patterns, remove their internal use Jan 20, 2025
@smelc smelc force-pushed the smelc/remove-patterns branch from 0e25624 to 7f8d417 Compare January 20, 2025 10:49
@smelc smelc added this pull request to the merge queue Jan 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 20, 2025
@carbolymer carbolymer merged commit f80706b into master Jan 20, 2025
29 checks passed
@carbolymer carbolymer deleted the smelc/remove-patterns branch January 20, 2025 16:39
@smelc smelc mentioned this pull request Jan 22, 2025
2 tasks
smelc added a commit to IntersectMBO/cardano-cli that referenced this pull request Jan 23, 2025
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