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

[CDEC-405] #3175

Merged
merged 3 commits into from
Jul 3, 2018
Merged

[CDEC-405] #3175

merged 3 commits into from
Jul 3, 2018

Conversation

coot
Copy link
Contributor

@coot coot commented Jul 2, 2018

Description

This solves the recent problem with lib/configuration.yaml not being parsable anymore. This happend because core team rightly removed some partial accessor functions. I just updated the yaml file and I alsoI added a test. This is a short term solution, long term I think we should write FromJSON instances for the types we are reading, because just removing fields from config file will make it less and less readable. @erikd it's up to you to decide if this maintaining FromJSON instances is worth the effort vs readability of the config file. I guess this really depends how many more partial functions we will remove.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CDEC-405

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.

Testing checklist

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

QA Steps

Screenshots (if available)

@coot coot requested review from erikd, avieth and parsonsmatt July 2, 2018 18:11
erikd
erikd previously requested changes Jul 3, 2018
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.

If you revert the changes to lib/configuration.yaml then i think this is good to go.

file: mainnet-genesis.json
hash: 5f20df933584822601f9e3f8c024eb5eb252fe8cefb24d1317dc3d432e940ebb
- mainnet-genesis.json
- 5f20df933584822601f9e3f8c024eb5eb252fe8cefb24d1317dc3d432e940ebb
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that is wrong, or rather that there should not be any change to that file. Commit b258f92 changed the way the config was parsed and has since been reverted.

You can probably drop these changes to this file and still have the test pass (as long as you have rebased).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, let me try!

@coot
Copy link
Contributor Author

coot commented Jul 3, 2018

@erikd, @parsonsmatt, @avieth any chance for taking a look at this tiny PR.

coot added 3 commits July 3, 2018 10:46
Like `withConfigurations` but takes explicit `LoggerName` argument.
This is useful when one wants to run a computation in `IO` where
`WithLogger` instance is not available.
@coot coot force-pushed the coot/cdec-405 branch from 76db77d to 0d7cde2 Compare July 3, 2018 09:05
@erikd erikd dismissed their stale review July 3, 2018 09:18

Fixed

@coot coot changed the title Coot/cdec 405 [CDEC-405] Jul 3, 2018
@coot coot merged commit ceca39e into develop Jul 3, 2018
@coot coot deleted the coot/cdec-405 branch July 25, 2018 07:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants