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

Improve our use of gofmt. #4379

Merged
merged 5 commits into from Mar 27, 2017

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Mar 24, 2017

Previously we integrated as a checkstyle-type check, that only logged
errors but didn't correct them. In fact, it didn't even do that, it
just logged the names of files that had style errors (and even that
in a convoluted way).

Now the checkstyle task actually reports the errors it found.

Also adds a fmt task that actually fixes the errors.

Improve our use of gofmt.
Previously we integrated as a checkstyle-type check, that only logged
errors but didn't correct them.  In fact, it didn't even do that, it
just logged the names of files that had style errors (and even that
in a convoluted way).

Now the checkstyle task actually reports the errors it found.

We also add a fmt task that actually fixes the errors.

@benjyw benjyw requested review from jsirois and JieGhost Mar 24, 2017

@stuhood

Thanks.

@@ -45,5 +46,6 @@ def register_goals():
task(name='go', action=GoCompile).install('compile')
task(name='go', action=GoBinaryCreate).install('binary')
task(name='go', action=GoRun).install('run')
task(name='gofmt', action=GoFmt).install('compile')
task(name='go-checkstyle', action=GoCheckstyle).install('compile')

This comment has been minimized.

@stuhood

stuhood Mar 24, 2017

Member

This is technically a breaking change... can you add a deprecated scope for this task?

@stuhood

stuhood Mar 24, 2017

Member

This is technically a breaking change... can you add a deprecated scope for this task?

This comment has been minimized.

@benjyw

benjyw Mar 24, 2017

Contributor

Done!

@benjyw

benjyw Mar 24, 2017

Contributor

Done!

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 24, 2017

Contributor

Also fixed some dep issues by merging all the micro-targets into per-package targets. We need to keep moving away from target-per-file...

Contributor

benjyw commented Mar 24, 2017

Also fixed some dep issues by merging all the micro-targets into per-package targets. We need to keep moving away from target-per-file...

Show outdated Hide outdated contrib/go/src/python/pants/contrib/go/tasks/go_fmt.py
if output:
self.context.logger.info(output)
def calculate_sources(self, targets):

This comment has been minimized.

@wisechengyi

wisechengyi Mar 25, 2017

Contributor

I understand the execute body could be a bit different between GoFmt and GoCheckstyle, but do you think calculate_sources and _is_checked can be pushed up to GoWorkspaceTask?

@wisechengyi

wisechengyi Mar 25, 2017

Contributor

I understand the execute body could be a bit different between GoFmt and GoCheckstyle, but do you think calculate_sources and _is_checked can be pushed up to GoWorkspaceTask?

This comment has been minimized.

@benjyw

benjyw Mar 26, 2017

Contributor

Ah good idea. This was a bit of a Friday afternoon rush job. Changed this to share almost all the code.

@benjyw

benjyw Mar 26, 2017

Contributor

Ah good idea. This was a bit of a Friday afternoon rush job. Changed this to share almost all the code.

@wisechengyi

Thanks!

@benjyw benjyw merged commit 3b1f84d into pantsbuild:master Mar 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:gofmt branch Mar 27, 2017

peiyuwang added a commit to peiyuwang/pants that referenced this pull request Mar 29, 2017

Improve our use of gofmt. (#4379)
Previously we integrated as a checkstyle-type check, that only logged
errors but didn't correct them.  In fact, it didn't even do that, it
just logged the names of files that had style errors (and even that
in a convoluted way).

Now the checkstyle task actually reports the errors it found.

We also add a fmt task that actually fixes the errors.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Improve our use of gofmt. (#4379)
Previously we integrated as a checkstyle-type check, that only logged
errors but didn't correct them.  In fact, it didn't even do that, it
just logged the names of files that had style errors (and even that
in a convoluted way).

Now the checkstyle task actually reports the errors it found.

We also add a fmt task that actually fixes the errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment