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 Go unconvert checker #905

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dominikh
Member

dominikh commented Mar 12, 2016

No description provided.

@dominikh

This comment has been minimized.

Member

dominikh commented Mar 12, 2016

On second thought I'm not sure if this checker should be added. It consumes a fair amount of CPU time:

$ cd go/src/net/http
$ time unconvert . 
unconvert .  3.28s user 0.22s system 442% cpu 0.790 total

That's probably too much to run regularly.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 14, 2016

@dominikh We could run it after the buffer is saved, though… not sure however, how that'd feel.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 14, 2016

Looking at the implementation, I think we need to do that anyway.

@dominikh

This comment has been minimized.

Member

dominikh commented Mar 14, 2016

Yes, it needs a saved buffer, much like errcheck does. I've checked, and errcheck has roughly the same performance characteristics:

$ time unconvert cmd/go
unconvert cmd/go  4.29s user 0.29s system 484% cpu 0.948 total

$ time errcheck cmd/go
errcheck cmd/go  4.62s user 0.28s system 473% cpu 1.036 total

So if we're fine with running errcheck, we're probably also fine with running unconvert(?)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 17, 2016

@dominikh Fine for me, absolutely. However, you need to change the unconvert checker to follow errchecks way of checking whether the buffer has a backing file and is saved. Would you mind to update this PR accordingly?

@dominikh dominikh closed this Mar 17, 2016

@dominikh dominikh force-pushed the dominikh:master branch from 15edd4a to df8e154 Mar 17, 2016

@lunaryorn lunaryorn removed the S-blocked label Mar 17, 2016

@dominikh

This comment has been minimized.

Member

dominikh commented Mar 17, 2016

Sigh, sorry for closing this, I force-pushed the wrong branch

@dominikh dominikh reopened this Mar 17, 2016

@dominikh

This comment has been minimized.

Member

dominikh commented Mar 17, 2016

It should be all good now. I added the predicate (and cleaned up errcheck's predicate in the process, a tiny leftover from the previous PR).

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 22, 2016

@dominikh Thank you. I'll review soon, but I'm a bit out of time currently, so please be patient ☺️

@lunaryorn lunaryorn added the S-ready label Mar 31, 2016

@lunaryorn lunaryorn closed this in 53dc5ad Apr 28, 2016

@lunaryorn lunaryorn removed the S-ready label Apr 28, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 28, 2016

@dominikh I've finally merged this one. Sorry for the delay, and many thanks for the contribution.

Would you like to join our team to look after and take care of Flycheck's Go support? We'd give you commit access so that you can merge Go-related pull requests yourself and directly commit fixes to the Go support to master. /cc @cpitclaudel

@dominikh

This comment has been minimized.

Member

dominikh commented Apr 28, 2016

Sorry for the delay

Absolutely no problem.

Would you like to join our team to look after and take care of Flycheck's Go support

Sure, why not. Are there some guidelines regarding code review, labels / the general process?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 29, 2016

@dominikh Thank you for understanding.

I've sent you an invitation for our org. Once accepting it you should get commit access to our repository.

We have a section for contributors in our documentation. The Contributor's Guide explains our guidelines for commits/PRs and the Github labels that we use.

There's also a guide for maintainers but it's still very much in progress. But I'll try to add guidelines for reviewing and merging PRs soon.

Generally we do everything per pull request, except for smaller fixes (e.g. typos, obvious bugs, etc.). When merging PRs we tend to cherry pick commits when the pull request is small—like this one—and only do proper merges when there's a really large feature with many commits coming in, where we should preserve the history of the feature and provide single point to revert the entire change in case of breakage.

If you're using Magit take a look at magit-gh-pulls which adds pull requests to Magit's status buffer and let's you fetch the corresponding commits into the local clone. It's my main interface to review pull requests–by looking at the commits and/or checking them out and testing the change–and to merge pull requests—I simply apply commits from a pull request to master, squash them, and amend them if necessary (mostly for doc improvements, and adding issue references to the commit message). Credit for this workflow goes to @syl20bnr who uses it for Spacemacs and told me about it.

I hope to get all of this information with more detailled instructions into the maintainer guide soon, but meanwhile that's it; we're not really doing a lot of magic here ☺️ Don't worry, we'll still review pull requests as well, so there's nothing that can go wrong 😊

Please ask if you have any further questions, and welcome on board! 👏

lunaryorn added a commit that referenced this pull request Apr 29, 2016

@dominikh

This comment has been minimized.

Member

dominikh commented Apr 29, 2016

Okay, the workflow sounds largely similar to mine (except for more CLI magic and less magit), so that's great. I assume that contributors need peer review on their PRs before merging them as well, assuming the change justifies a PR?

and welcome on board!

Thanks!

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 29, 2016

I assume that contributors need peer review on their PRs before merging them as well, assuming the change justifies a PR?

Yes, please. I'd like to take at least a cursory look at each pull request ☺️

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 29, 2016

👍 🎉 Welcome aboard :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment