Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Log json exception details on chain config file load error #5526

Merged
merged 3 commits into from
Mar 28, 2019

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Mar 23, 2019

Also return new error code AlethErrors::ConfigFileInvalid

@codecov-io
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

Merging #5526 into master will increase coverage by 0.02%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5526      +/-   ##
==========================================
+ Coverage   61.86%   61.88%   +0.02%     
==========================================
  Files         344      344              
  Lines       28755    28763       +8     
  Branches     3267     3267              
==========================================
+ Hits        17789    17800      +11     
  Misses       9797     9797              
+ Partials     1169     1166       -3

@gumb0
Copy link
Member

gumb0 commented Mar 25, 2019

I would prefer to keep json_spirit dependency inside ChainParams.cpp, i.e. handle js::Error_position in ChainParams::loadConfig and re-throw as dev::Exception-derived exception.

(There is already some validation of json in loadConfig, that does this (see validateConfigJson). Also it logs directly to std::error, I think you may do the same)

@halfalicious
Copy link
Contributor Author

at does this (see validateConfigJson). Also it logs directly to std::error, I think you may do the same)

Makes sense, thanks for the feedback!

@halfalicious halfalicious force-pushed the private-chain branch 3 times, most recently from 8fbe95d to 3769436 Compare March 27, 2019 04:36
@halfalicious
Copy link
Contributor Author

Rebased (to get changelog) and updated changelog

@halfalicious
Copy link
Contributor Author

Looks like there's a unit test that needs updating

@halfalicious
Copy link
Contributor Author

This test is failing on osx but it's a test reliability issue since my changes don't touch the discovery part of the code:

510 - network/net/packetsWithChangedEndpointSuite/neighbours (Failed)

I'll open a new issue so we can track this.

@halfalicious
Copy link
Contributor Author

Rebased to fix changelog

@halfalicious halfalicious merged commit 523c965 into master Mar 28, 2019
@halfalicious halfalicious deleted the private-chain branch March 28, 2019 02:57
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.

3 participants