-
Notifications
You must be signed in to change notification settings - Fork 46
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
editoast: move Generate and ImportRailjson to infra subcommand #5722
editoast: move Generate and ImportRailjson to infra subcommand #5722
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #5722 +/- ##
=========================================
Coverage 19.53% 19.53%
Complexity 2332 2332
=========================================
Files 907 907
Lines 106998 107014 +16
Branches 2593 2593
=========================================
+ Hits 20901 20907 +6
- Misses 84571 84581 +10
Partials 1526 1526
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dc3dc5f
to
310259e
Compare
310259e
to
7ed69d4
Compare
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. A few changes were requested.
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 answered on @flomonster comments.
For consistency, also replace the eprintln!
s of this file by an Err(CliError...)
where appropriate.
LGMT otherwise :)
7ed69d4
to
3caba0d
Compare
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.
LGTM. I made some suggestions but overall, it looks good
3caba0d
to
bedb0dd
Compare
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.
LGTM. Not tested since my last review.
closes #5699