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

Feature/issue history - make retrieval of history dependent on GetHistory argument/parameters switch #441

Open
wants to merge 2 commits into
base: release/2.15-alpha
Choose a base branch
from

Conversation

GeekTieGuy
Copy link

Description

Introduce the argument/parameter GetHistory to allow control of fetching issue history at call site

Motivation and Context

Getting the history is very useful, but it would be even better if it can be a choice at the call site.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added Pester Tests that describe what my changes should do.
  • I have updated the documentation accordingly.

@GeekTieGuy GeekTieGuy requested review from a team as code owners July 15, 2021 19:14
@GeekTieGuy
Copy link
Author

Is there any chance this will get picked up? I'm not familiar with the community/process, but I think the change is useful and fairly cleanly implemented.

Copy link
Member

@lipkau lipkau left a comment

Choose a reason for hiding this comment

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

Sorry that it took me so long to review this PR.

with the new CI setup, you can see which tests are failing.
But I have pointed out the most important things that are missing above.

Copy link
Member

Choose a reason for hiding this comment

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

This file is missing documentation docs/en-US/commands/Get-JiraStatus.md and tests Tests/Functions/Get-JiraStatus.Unit.Tests.ps1.

Copy link
Member

Choose a reason for hiding this comment

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

Tests/Functions/Get-JiraIssue.Unit.Tests.ps1 needs to be extended with this new functionality.

Copy link
Member

Choose a reason for hiding this comment

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

can you elaborate on why this change is necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I don't remember precisely anymore, because it's been a long time since I did this work. I think it had to do with the "toString" identifier colliding with a .NET method, and this was the simplest way I could think of to avoid that collision.

param(
[Parameter( Mandatory, ValueFromPipeline, ParameterSetName = '_Search' )]
[String[]]
$IdOrName,
Copy link
Member

Choose a reason for hiding this comment

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

Please rename variable to $Status to be analog to similar implementations.
eg JiraPS/Public/Get-JiraField.ps1

The documentations should elaborate on what values are allowed to be used for the parameter.

@lipkau lipkau changed the base branch from master to release/2.15-alpha June 13, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants