Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

update all dependencies #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crfeliz
Copy link

@crfeliz crfeliz commented Apr 22, 2019

#45
Update all dependencies. Add .idea and issue-sync (output of go build) to .gitignore. Specify int64 where necessary.

@crfeliz crfeliz mentioned this pull request Apr 22, 2019
…go build) to .gitignore. Specify int64 where necessary.
@crfeliz
Copy link
Author

crfeliz commented Apr 24, 2019

@squat I know this is quit a large change set. What it boils down to is running a glide upgrade and changing int to int64 in a few places. Could you offer some guidance on how to get this upgrade approved for upstream? I have a few other feature-based changes I hope to push up as well however this upgrade is prerequisite.

@crfeliz
Copy link
Author

crfeliz commented May 7, 2019

Bumping this issue. Let me know if there is anything I can do to to get this change review/merged. I know i looks daunting but this is pretty much the result of running glide update and then fixing the few errors int64 related errors. If you have a preferred plan of action for updating dependencies I'd be happy to follow that process.

Copy link
Contributor

@squat squat left a comment

Choose a reason for hiding this comment

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

Generally looks good. Two small questions and we can merge!

@@ -14,3 +14,5 @@
.glide/

bin/
.idea/
issue-sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this should not be here. All binaries should be compiles directly in `bin. If not, we have some build system bug

Copy link
Author

Choose a reason for hiding this comment

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

yea it currently builds in the same directory as source. I can look into that. I'll remove this from the pr.

@@ -14,3 +14,5 @@
.glide/

bin/
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for some editor files?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I use IntelliJ which creates editor files under that directory.

@@ -56,7 +56,7 @@ func CompareComments(config cfg.Config, ghIssue github.Issue, jIssue jira.Issue,
}
// matches[0] is the whole string, matches[1] is the ID
matches := jCommentIDRegex.FindStringSubmatch(jComment.Body)
id, _ := strconv.Atoi(matches[1])
id, _ := strconv.ParseInt(matches[1], 10, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not from this PR,but we need to give this module some TLC and check all returned errors.

Copy link
Author

Choose a reason for hiding this comment

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

Yep. My upcoming PRs add features as well as more thorough error handling and I'm currently working on unit tests. It might prove difficult to slice the upcoming work into small PRs (I'll do my best obviously) but included in those changes are some minor improvements in various parts of the code.

@squat
Copy link
Contributor

squat commented May 11, 2019 via email

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

Successfully merging this pull request may close these issues.

2 participants