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 Travis, Badges, and fixed all linting errors (RC 2.0.0) #18

Merged
merged 1 commit into from
Dec 27, 2018
Merged

Add Travis, Badges, and fixed all linting errors (RC 2.0.0) #18

merged 1 commit into from
Dec 27, 2018

Conversation

king-jam
Copy link
Contributor

@king-jam king-jam commented Dec 21, 2018

You'll need to enable https://travis-ci.com on the repo (it is free because it's a public repo) but then that feature should start working.

Closes: #17

@king-jam
Copy link
Contributor Author

Should I roll #6 into this as well since all breaking API changes should probably get captured here?

Copy link
Contributor

@tchap tchap left a comment

Choose a reason for hiding this comment

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

Not sure why the license headers are being removed...

v5/pivotal/cursor.go Show resolved Hide resolved
v5/pivotal/errors.go Outdated Show resolved Hide resolved
v5/pivotal/iterations.go Outdated Show resolved Hide resolved
v5/pivotal/memberships.go Outdated Show resolved Hide resolved
v5/pivotal/projects.go Outdated Show resolved Hide resolved
@tchap
Copy link
Contributor

tchap commented Dec 22, 2018

@king-jam You can add #6 in case @iterion doesn't mind...

@king-jam
Copy link
Contributor Author

king-jam commented Dec 22, 2018

@tchap license headers in that format actually violate the linters. If you move the license headers into a COPYING or as an additional LICENSE file, those satisfy legal requirements of copyright protection (I've dealt with open sourcing things a few times with my company's legal team). They were also sparsely placed. Do you want them everywhere?

@tchap
Copy link
Contributor

tchap commented Dec 22, 2018

@king-jam If you know without searching how to do it, could you just copy what is necessary into COPYING? Otherwise I will do something about this when merged...

@king-jam
Copy link
Contributor Author

I was looking at the linting errors more closely and it just required a newline between the header and the package declaration. I opted to just update them in place. I agree that a change like that should be a separate commit and more planned.

@king-jam
Copy link
Contributor Author

I think tweaking #6 should wait until after this merge. Cutting a release locks in Glide/Dep/go mod/etc so tweaking the 2.0.0 interface doesn't need to be rushed in a large giant commit.

@tchap
Copy link
Contributor

tchap commented Dec 23, 2018

@king-jam So are we good to merge or is there anything else left? Thanks for the effort.

@king-jam
Copy link
Contributor Author

I think we are good to merge.

@tchap
Copy link
Contributor

tchap commented Dec 26, 2018

@king-jam I am wondering about naming still, there are many fields named like TaskIds, i.e. we are using ID and Ids. Should we simply use IDs?

@king-jam
Copy link
Contributor Author

king-jam commented Dec 26, 2018

If there are, I absolutely missed them and will need to fix. Seems like plural versions pass linters.

@tchap
Copy link
Contributor

tchap commented Dec 27, 2018

Looks good. Do you think that you could rebase and squash what makes sense to get nicer commits? Like, we definitely don't need what was committed and reverted again. The badge commits can be merged together as well. Thanks!

@king-jam
Copy link
Contributor Author

Definitely. Was going to with that last commit but didn't want to lose any of the simpler diffs.

@king-jam
Copy link
Contributor Author

Squashed!

@tchap tchap merged commit 6d1834b into salsita:master Dec 27, 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.

2 participants