REFACTOR: Remove ImageBuilder struct #7682
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
christophermaier
commented
May 12, 2020
christophermaier
commented
As these refactorings have progressed, `ImageBuilder` has been revealed to be the equivalent of Tom Smykowski in the movie Office Space. Now, the `Naming` struct generates the "expanded identifiers" that `ImageBuilder` was via the `Identified` trait. This makes more sense, in that all the identifier logic is consolidated in the `Naming` struct. A future commit will remove the `Identified` trait altogether, thus fulfilling the prophecy foretold in 70a8aca. Signed-off-by: Christopher Maier <cmaier@chef.io>
This allows us to get rid of the `Identified` trait entirely. Signed-off-by: Christopher Maier <cmaier@chef.io>
davidMcneil
approved these changes
May 13, 2020
| pub fn image_identifiers(&self, | ||
| ident: &FullyQualifiedPackageIdent, | ||
| channel: &ChannelIdent) | ||
| -> Result<(String, Vec<String>)> { | ||
| -> Result<(String, Vec<String>, Vec<String>)> { |
There was a problem hiding this comment.
Would it be worth having a type alias or maybe even a new type for the different types of tags?
There was a problem hiding this comment.
Maybe... writing this return type killed a little bit of my soul.
I think that with some additional future refactorings this would get a lot simpler, but it may be a good idea to contain the gnarliness until then.
There was a problem hiding this comment.
Addressed in f057ddb; much better 😄
| } | ||
|
|
||
| impl ContainerImage { | ||
| // TODO (CM): temporary; we shouldn't use this at all | ||
| pub fn workdir(&self) -> &Path { self.workdir.as_path() } | ||
|
|
||
| pub fn expanded_identifiers(&self) -> &Vec<String> { &self.expanded_identifiers } |
There was a problem hiding this comment.
Can we make the return type &[String]?
There was a problem hiding this comment.
Addressed in decc378
Signed-off-by: Christopher Maier <cmaier@chef.io>
Signed-off-by: Christopher Maier <cmaier@chef.io>
christophermaier
force-pushed
the
cm/remove-image-builder
branch
from
f057ddb
to
d470028
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.