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 'priority' field to formatter #64

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

stfn775
Copy link
Contributor

@stfn775 stfn775 commented Sep 27, 2023

The priority field was missing therefore not reported by the "get_issue" action.

@stfn775 stfn775 requested review from floatingstatic and a team as code owners September 27, 2023 16:47
@stfn775 stfn775 requested review from arm4b and removed request for a team September 27, 2023 16:47
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@floatingstatic floatingstatic left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Looks good over all. We should definitely add this to the output, just one comment/question before we merge. Thanks!

@@ -34,6 +34,7 @@ def to_issue_dict(issue, include_comments=False, include_attachments=False,
'summary': issue.fields.summary,
'description': issue.fields.description,
'status': issue.fields.status.name,
'priority': issue.fields.priority,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I suggest using issue.fields.priority.name here instead of just issue.fields.priority? If we leave this as is the response ends up looking something like this in the response object:

'priority': <JIRA Priority: name='Medium', id='3'>,

This makes it kinda hard to parse things since we are basically returning a jira.resource.Priority type, but I suspect what you are probably after mostly is the human readable "name" for the priority (in my example "Medium"). Does that work for you or break your particular use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My use case works with either name, id or I can work with the whole object, no preference.
I left it as type in the PR as it seems name is a customisable fields and it might change, so some use cases might prefer to use the id field instead.
I agree with you though, returning the name field makes it more user friendly and ease the implementation, so I would go with that.
Should I change it in the PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking for other people consuming this pack it would be best to use issue.fields.priority.name. This is also consistent with what we have done with other fields (status, for example).

You can update this in your branch directly and it will be reflected in the PR. Once that is done I can +1 the PR and get it merged. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for taking the time to review!

@floatingstatic floatingstatic merged commit 7ba2014 into StackStorm-Exchange:master Sep 28, 2023
1 check passed
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.

3 participants