-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Chainspec validation #6197
Chainspec validation #6197
Conversation
According to gitlab, rust job succeeded. It was just not reported here |
scripts/validate_chainspecs.sh
Outdated
ERR=0 | ||
cargo build --release -p chainspec | ||
|
||
for spec in `ls ethcore/res/*.json`; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary ls
, just do for spec in path/to/dir/*; do
. Iterating over ls
output is unstable anyway. Also, backticks are deprecated and also smell and are bad.
Hey, instead of me telling you this stuff, go install Shellcheck since I think both of those notes are implemented as lints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! I will check it out :D
chainspec/src/main.rs
Outdated
use std::{fs, env, process}; | ||
use ethjson::spec::Spec; | ||
|
||
fn quit<S: AsRef<str>>(s: S) -> ! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Foo: AsRef<Bar>
as a type parameter is unnecessary because of Rust's automatic coercion (for example, foo(&bar)
will succeed if foo
takes a &T
and bar
implements AsRef<T>
). I think I've made this same mistake in my latest PR with AsRef<Path>
. So this could just take &str
with the same API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got some nits but I'd be happy with this being merged. You can fix them up if you like, but it's not a problem.
But we already have a unit test that does that. See #6028 |
So there are few reasons to actually have
2 && 3 can be addressed with improved unit tests, but 1 not |
@arkpar I removed existing tests |
Agreed with @debris on this -- the broken chainspecs in the repo were all not covered by the tests when I made my PR. |
changes:
scrips/validate_chainspec.sh
which validates our chainspecsvalidate_chainspec.sh
output looks like this: