Skip to content
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

Add feature buildtest buildspec find --row-count #1377

Merged
merged 4 commits into from
Feb 9, 2023

Conversation

prathmesh4321
Copy link
Collaborator

Hi @shahzebsiddiqui. I have added the "--row-count" feature for "buildtest buildspec find". Please review the PR.

Screen Shot 2023-02-07 at 7 45 08 PM

Screen Shot 2023-02-07 at 7 45 49 PM

@prathmesh4321 prathmesh4321 linked an issue Feb 8, 2023 that may be closed by this pull request
2 tasks
@shahzebsiddiqui
Copy link
Member

@prathmesh4321 please try this implementation for

buildtest bc find -t
buildtest bc find -e
buildtest bc find -b

this is not relevant for -p since its just paths to root of buildspecs.

I dont think it makes sense to have --row-count for --group-by-tags and --group-by-executor

I am not sure if its useful to know total number of filter fields and format fields, if its important then potentially we add support with option --filterfields and --formatfields

@shahzebsiddiqui shahzebsiddiqui linked an issue Feb 8, 2023 that may be closed by this pull request
6 tasks
@prathmesh4321
Copy link
Collaborator Author

@prathmesh4321 please try this implementation for

buildtest bc find -t
buildtest bc find -e
buildtest bc find -b

this is not relevant for -p since its just paths to root of buildspecs.

I dont think it makes sense to have --row-count for --group-by-tags and --group-by-executor

I am not sure if its useful to know total number of filter fields and format fields, if its important then potentially we add support with option --filterfields and --formatfields

@shahzebsiddiqui Okay, I have added --row-count option for "buildtest bc find -t", "buildtest bc find -e", "buildtest bc find -b". I don't think its useful to add --row-count option for --filterfields and --formatfields.

Screen Shot 2023-02-08 at 12 24 06 PM

Copy link
Member

@shahzebsiddiqui shahzebsiddiqui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@shahzebsiddiqui shahzebsiddiqui merged commit 18b4677 into devel Feb 9, 2023
@shahzebsiddiqui shahzebsiddiqui deleted the buildtest_bc_find_row_count branch February 9, 2023 21:23
@jscook2345
Copy link
Collaborator

Rather than adding this as an option to all of your print functions I would consider instead checking for the argument in the main function and than printing the table there since all of the code is the same, but copied to different places.

@shahzebsiddiqui
Copy link
Member

@jscook2345 so you want a top level argument buildtest --row-count instead of implementing it in the subcommands.

Currently this is present in buildtest report and buildtest buildspec find.

I thought about it when there is redundancy I think it does make sense to move this to top level argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: buildtest buildspec find --buildspec-count [FEATURE]: buildtest buildspec find --row-count
3 participants