Add products command #516
Merged
Add products command #516
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
* Lists available, staged, and deployed products * Replaces 'available-products', 'staged-products', and 'deployed-products' commands * Update docs around products command * Add deprecation notices to 'available-products', 'staged-products', and 'deployed-products' commands
|
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story. The labels on this github issue will be updated when the story is started. |
|
Supports any combination of |
|
There are probably some edge cases that need to be thought through. |
|
We just tried with With When we run it with the command There is a difference of:
|
|
Cool, I'll look into those and push a change with the fixes! |
|
Keep in mind the combined view @eitansuez linked in the issue for as well (the empty line so that everything organizes nice) |
- Hide products if they have no versions that satisfy the requested columns - Display multiple available product versions
|
Here's the updated output for And |
|
Seems like I dropped the |
|
All Comparison outputs (for documentation purposes): It looks real good (in terms of output) IMO. It takes the same display and functionality, and makes it easier to read and look at. Thanks for alphabetizing the products, too. that adds a lot. Looking at the commits now |
kcboyle
reviewed
Oct 15, 2020
| productVersionsCombiner[product.Name] = productVersions | ||
| } | ||
|
|
||
| for _, product := range deployedProducts { |
There was a problem hiding this comment.
This whole section is clear, and easy to read. If I'm trying to be nitpicky, I'd love to see these for loops in functions (combineStagedProducts, combineAvailableProducts, combineDeployedProducts or something similar)
Just to let this flow top->bottom a little faster without the for loop break
There was a problem hiding this comment.
if these functions also have the sp receiver like Execute, you can also clean up some of the earlier setup
There was a problem hiding this comment.
The only reason I didn't pull these into separate functions was the complexity of their signatures when using Intellij's extract method feature. It might not have actually needed to be that complicated, though. I just didn't dig further at that point.
There was a problem hiding this comment.
had a quick additional commit to implement this.
|
Added a couple more display check tests. Adding the outputs here for documentation
|
kcboyle
reviewed
Oct 15, 2020
| var outputData []string | ||
|
|
||
| moreAvailableProducts = false | ||
| if products.Available { |
There was a problem hiding this comment.
lost of nested ifs in these for loops.
This is still clear, but aesthetically could put the logic of these if blocks into functions to make the nesting a little less crazy
There was a problem hiding this comment.
Makes sense. I was definitely running short on time when doing this and wanted something working before the refactoring step.
There was a problem hiding this comment.
looking into this now, and refactoring is a little more complicated. We looked into it and are not going to refactor. Maybe there's a good way to do it, but not as simple as originally thought
|
Added a couple refactor suggestions in the commits @iplay88keys Thank you for re-generating docs 👍 |
|
Merged |
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.
Resolves #466