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

conform imports to schema defined in CONTRIBUTING.md #150

Merged
merged 4 commits into from
Mar 14, 2020

Conversation

laser
Copy link
Contributor

@laser laser commented Mar 13, 2020

Why does this PR exist?

Our imports don't match what's in the style guide:

### Conventions and Style

#### Imports
We use the following import ordering.

import (
        [stdlib packages, alpha-sorted]
        <single, empty line>
        [external packages]
        <single, empty line>
        [go-fil-markets packages]
)

What's in this PR?

This PR replaces the single-line imports with import-groupings, and clustered go-fil-markets imports as per CONTRIBUTING.md.

How do I achieve this awesome style myself?

On an OSX box, do this:

find . -type f -name \*.go | xargs -I '{}' goimports -w -local "github.com/filecoin-project/go-fil-markets" '{}'

then do this:

find . -type f -name \*.go | xargs -I '{}' sed -i.bak -e '/import (/ {
  :1
  $!N
  s/\n\n/\'$'\n''/
  /)/!b1
}' '{}'

then do this:

cd extern/filecoin-ffi ; git checkout . ; git clean -fd ; cd ../.. ; git checkout . ; git clean -fd

and finally:

find . -type f -name \*.go | xargs -I '{}' goimports -w -local "github.com/filecoin-project/go-fil-markets" '{}'

@laser laser requested review from hannahhoward and shannonwells and removed request for hannahhoward March 13, 2020 22:44
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

In the original styleguide for go-filecoin, everything was all one filecoin repo.

I have been working under the assumption that if the repo is in filecoin-project, it should be in the last group. That’s why you may find many non-fil-markets deps in the bottom block. I realize the styleguide as written does not make that clear.

I guess I have no objection to changing it. LGTM unless you think it's worthy of further conversation.

@laser
Copy link
Contributor Author

laser commented Mar 13, 2020

@hannahhoward

Makes sense. Would you mind approving? I will merge.

@hannahhoward hannahhoward self-requested a review March 13, 2020 23:06
@hannahhoward
Copy link
Collaborator

sorry I thought I had

@laser laser removed the request for review from shannonwells March 14, 2020 00:12
@laser laser merged commit 76144be into master Mar 14, 2020
@laser laser deleted the chores/conform-imports-to-style-guide branch March 14, 2020 00:12
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