Conversation
Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
akihikodaki
left a comment
There was a problem hiding this comment.
Assuming the only difference between listtestresults and listallresults is that the presence of $target, I think it's better to handle the case that $target is not specified for listtestresults instead of adding a completely new function.
This will break backward compatibility because
|
It seems positional parameters can still be explicitly named: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_functions_advanced_parameters?view=powershell-7.5 So the following command should work: listtestresults -target <targetkey> -project <projectname> -machine <testmachine> -pool <poolname>However, it will lead to the following statement: throw "Please provide a test's id."We can implement a logic to handle the case where the test ID is missing here. |
|
I suggest not to increase a complexity for the future developers. Better have additional function. Return list of lists is also different from returning a list. |
|
I expect explicitly naming parameters results in better readability than having an additional function. It is not clear how Both functions return an array of objects. |
|
What about deduplicate code but still have 2 functions the first with test_id, and the second without? |
The code duplication is problematic, but I also see one function with optionally named parameters is better as an interface design than two similar functions due to the reason described in: #63 (comment) |
Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
d265d73 to
2905787
Compare
In this case, all results will be listed. Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
No description provided.