Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CDEC-369] Remove HasProtocolConstants in favour of parameters #3482

Merged
merged 1 commit into from
Aug 28, 2018
Merged

[CDEC-369] Remove HasProtocolConstants in favour of parameters #3482

merged 1 commit into from
Aug 28, 2018

Conversation

ruhatch
Copy link
Contributor

@ruhatch ruhatch commented Aug 24, 2018

Description

This is a revival of this PR

Linked issue

CDEC-369

Type of change

  • [~] 🐞 Bug fix (non-breaking change which fixes an issue)
  • [~] 🛠 New feature (non-breaking change which adds functionality)
  • [~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • [~] 🔨 New or improved tests for existing code
  • [~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • [~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • [~] I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@ruhatch ruhatch mentioned this pull request Aug 24, 2018
Closed
5 tasks
@ruhatch ruhatch force-pushed the ruhatch/CDEC-509 branch 2 times, most recently from 3135c89 to 39c6899 Compare August 27, 2018 17:08
@ruhatch ruhatch requested review from avieth and mhuesch August 27, 2018 19:57
erikd
erikd reviewed Aug 28, 2018
View reviewed changes
core/test/Test/Pos/Core/EnumEmpTest.hs
-- on the `HasProtocoConstants` constraint. Since we want to remove
-- `HasProtocoConstants` the first step is to provide functions that will act
-- like the `Enum` methods, but take an extra parameter for
-- the part they require from `ProtocolConstants`.
Copy link
Member

Choose a reason for hiding this comment

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

Goodbye my lovely tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They served their purpose honourably!

erikd
erikd approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

LGTM!

@erikd erikd merged commit 7683f28 into develop Aug 28, 2018
erikd added a commit that referenced this pull request Aug 29, 2018
@erikd
Fix bad Github merge
ff279b8 
The changes in PRs #3491 and #3482 turned out to be incompatible.
To avoind things like this, it would be ideal if PRs were not
merged unless they were rebased and had a complete CI build first.
@erikd erikd mentioned this pull request Aug 29, 2018
Merged
12 tasks
erikd added a commit that referenced this pull request Aug 29, 2018
@erikd
Fix bad Github merge
0092424 
The changes in PRs #3491 and #3482 turned out to be incompatible.
To avoind things like this, it would be ideal if PRs were not
merged unless they were rebased and had a complete CI build first.
KtorZ pushed a commit that referenced this pull request Nov 9, 2018
@erikd
Merge pull request #3482 from input-output-hk/ruhatch/CDEC-509
7686dfc 
[CDEC-369] Remove HasProtocolConstants in favour of parameters
KtorZ pushed a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018
@erikd
Merge pull request input-output-hk/cardano-sl#3482 from input-output-…
eeb8181 
…hk/ruhatch/CDEC-509

[CDEC-369] Remove HasProtocolConstants in favour of parameters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikd erikd erikd approved these changes

@parsonsmatt parsonsmatt Awaiting requested review from parsonsmatt

@avieth avieth Awaiting requested review from avieth

@mhuesch mhuesch Awaiting requested review from mhuesch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruhatch @erikd