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

Tempo Timesheets woklogs #133

Closed

Conversation

smgladkovskiy
Copy link

Hi!

In my work I need to collect users worklogs. Native Jira methods did not give this information in clean and easy way. But there is a Tempo Timesheets plugin which has an API method to collect worklogs by username and dates.

I implemented method to get worklogs. Here it is.

Copy link
Owner

@andygrunwald andygrunwald 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 your contribution!

I wrote a couple of comments. And in general, the document strings need to be updated here.
godoc will complain with the current version

README.md Outdated
@@ -206,7 +206,7 @@ func main() {
AuthURL: fmt.Sprintf("%s/rest/auth/1/session", base),
}

jiraClient, err := jira.NewClient(tp.Client(), base)
jiraClient, _ := jira.NewClient(tp.Client(), base)
Copy link
Owner

Choose a reason for hiding this comment

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

You dropped err, because it is not used in the example, right?
If yes, i would go for an additional

if err != nil {
	panic(err)
}

here, because it is an example and is really often used as a copy / paste go to start code.
With the additional panic, people might start think about error handling.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will return it

jira.go Outdated
@@ -231,6 +234,10 @@ func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) {
// Open a NewDecoder and defer closing the reader only if there is a provided interface to decode to
defer httpResp.Body.Close()
err = json.NewDecoder(httpResp.Body).Decode(v)

if err != nil {
log.Printf("error JSON encoding body: %v", httpResp.Body)
Copy link
Owner

Choose a reason for hiding this comment

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

This leads to a side effect and is not necessary, because a couple of lines after in https://github.com/andygrunwald/go-jira/blob/master/jira.go#L246 the err is returned.
I would vote for a removal. Or is there any special reason for it?

Copy link
Author

Choose a reason for hiding this comment

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

Very often I can't understand what kind of error jira thrown when I ask for some data. Logging the answer helped to investigate it. I think that this debugging approach will be helpful for others in interacting with jira.

jira.go Outdated
@@ -12,6 +12,7 @@ import (

"github.com/google/go-querystring/query"
"github.com/pkg/errors"
"log"
Copy link
Owner

Choose a reason for hiding this comment

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

See my comment below at log.Printf...

)

const TTWorklogTimeFormat = "2006-01-02T15:04:05.000"
const TTWorklogDateFormat = "2006-01-02"
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add comments here? With a link to the docs as well?

Copy link
Author

Choose a reason for hiding this comment

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

WorklogBean that returns worklog api uses this time format. Watch the Returns section: http://developer.tempo.io/doc/timesheets/api/rest/latest/#848933329
2018-05-20 10 46 24

worklog.go Outdated
const TTWorklogTimeFormat = "2006-01-02T15:04:05.000"
const TTWorklogDateFormat = "2006-01-02"

// SprintService handles sprints in JIRA Agile API.
Copy link
Owner

Choose a reason for hiding this comment

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

This seems to be a copy / paste doc

Copy link
Author

Choose a reason for hiding this comment

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

Yep. Sorry. I'll clean it up.

@smgladkovskiy
Copy link
Author

And in general, the document strings need to be updated here.
godoc will complain with the current version

@andygrunwald suggest please how to add readme about this functional. Because it is a JIRA plugin api request, I think that it must be some kind of "additional possibilities" section. What do you think?

@andygrunwald
Copy link
Owner

@rbriski What do you think about the implementation of this JIRA API Request here?

Copy link
Contributor

@rbriski rbriski left a comment

Choose a reason for hiding this comment

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

I'm appreciative of the work here but I don't think we should add plugins into the core library. It leads to confusion when those services don't work for everyone.

I would, however, like to talk about how to import plugins so you could write it in your own repo and then import the library along with go-jira and add the service.

I've opened an issue at #135 to do this. In the meantime, if this is time sensitive, just use your fork. However, if you have time @smgladkovskiy , I'd love to get your help figuring out how to handle plugins in a more extensible way.

@@ -32,7 +32,7 @@ func NewJiraError(resp *Response, httpError error) error {
jerr := Error{HTTPError: httpError}
err = json.Unmarshal(body, &jerr)
if err != nil {
httpError = errors.Wrap(errors.New("Could not parse JSON"), httpError.Error())
httpError = errors.Wrap(errors.New(fmt.Sprintf("Could not parse JSON from body %s", string(body))), httpError.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to do this just remove errors.New and use fmt.Errorf

@rbriski rbriski closed this Nov 7, 2018
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.

None yet

3 participants