-
Notifications
You must be signed in to change notification settings - Fork 1
Challenge repo init command #30
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
base: main
Are you sure you want to change the base?
Conversation
|
clarification: i do not care about code formatting feedback, such as the compiler warnings for the structs or the |
detjensrobert
left a comment
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.
Structure comments:
- So far, I've tried to keep the bulk of the logic for comands in their own folders, and
run()orchestrates which thing to call based on the command line flags (i.e.build::run()calls build_image() and then push_images() if--pushwas given). I think the*_init()functions and the struct definitions should go in their own folder/file to follow this. Thoughts from the rest of the team?
done |
|
Since this PR is so out of date with the |
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
2510f36 to
bdfd2bd
Compare
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
We already have an existing struct used to parse the rcds.yaml config, use that to build the example/interactive config instead of creating an almost-identical struct. So far this is only done for the example values / non-interactive init. Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Having this be a string is more user friendly than a number, and this branch was originally set up to do that. Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Blank and placeholder template files should be set explicitly by the user. Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
serde_yml turns out to be maintained weirdly (issues disabled, weird refactors) and is now archived anyway. _ng is what the community is now recommending. Signed-off-by: Robert Detjens <github@detjens.dev>
|
This is now ready for review again, spruced up and tested. |
| // FORMATTING NOTE: Some of these help messages cause rustfmt to silently | ||
| // fail to format this struct definition. Commenting out the marked | ||
| // help_message lines temporarily will let the formatting work. | ||
| // | ||
| // Ref: | ||
| // - https://github.com/rust-lang/rustfmt/issues/6687 | ||
| // - https://github.com/rust-lang/rustfmt/issues/3863 |
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.
Rustfmt currently has some issues with formatting this file due to line length limitations. The issues linked here have more context (rust-lang/rustfmt#6687, rust-lang/rustfmt#3863) -- TLDR rustfmt silently fails on really long lines/method chains and thus fails to format this file unless the lines marked with // too long to format are commented out.
@cacama-valvata and I discussed offline a while ago about this and he recommended that this is is documented in the Discord rather than a code comment here since this seems unprofessional. I disagree with that point and prefer to have it documented here so that it cannot get lost or forgotten and have to figure this all out again in the future, because this is a bug in rustfmt and not (personally) a code smell.
Do any of the rest of you have thoughts on if this should be documented and where?
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.
TIL this is a problem. I wonder how often it's happened to me and I never noticed before?
I tried format_strings with the nightly toolchain and it doesn't help here. I don't think it's worth breaking the strings manually, either. I think the status quo is fine even if the too long to format comments aren't necessary.
| .with_help_message( | ||
| "The username that the cluster will use to pull locally-built containers.", | ||
| ) |
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 would personally just use a regex to comment these all out at once so I could format. This is the only help message that's not contained on a single line. Not a big deal to change it manually and do that.
detjensrobert
left a comment
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.
Also a note: the placeholder/blank config structs could be done via the Default trait on the object, but only one of them can so I have not done that. Should one of them be the Default, and if so which?
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
Signed-off-by: Robert Detjens <github@detjens.dev>
b1e0a60 to
0a82506
Compare
Opening a premature PR on this so that I can get midway feedback and get most of my "TODO" questions answered this Sunday (not all of the TODOs are for y'all but most of them are, feel free to comment on any you want to give insight on). Please add your answers to the TODOs via comments in a review on this PR since I can't be there this Sunday