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.

Sign up for GitHub

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

Jump to bottom

[Makefile] Define make imports #2162

Merged
merged 3 commits into from
Jan 29, 2021
Merged

[Makefile] Define make imports #2162

merged 3 commits into from
Jan 29, 2021

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Nov 6, 2018

This PR defines make imports for running goimports on the source. It also enables it for the linter and formats the affected files.

wpaulino
wpaulino reviewed Nov 9, 2018
View reviewed changes
Makefile Outdated
@$(call print, "Fixing imports.")
goimports -w $(GOFILES_NOVENDOR)

fmt: imports
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports will also run gofmt, so should we just replace it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation, will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like goimports doesn't support code simplification, like we add here: #2118

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just run both in make fmt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in don't define make imports?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, agree it's better to keep all formatting tools under one target (make fmt).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, done!

@halseth
Copy link
Contributor Author

halseth commented Nov 13, 2018

Also included the commit from #2118 to run formatting with simplification.

@Roasbeef Roasbeef added code health Related to code commenting, refactoring, and other non-behaviour improvements P3 might get fixed, nice to have needs review PR needs review by regular contributors makefile labels Nov 14, 2018
@halseth halseth mentioned this pull request Dec 11, 2018
Merged
@halseth halseth force-pushed the make-goimports branch 2 times, most recently from 2099cda to 8ec6451 Compare January 9, 2019 09:16
@halseth
Copy link
Contributor Author

halseth commented Jan 9, 2019

This has some performance problems with the usage of go modules. Will defer for now.

Relevant: golang/go#27287

@halseth halseth added the blocked label Jan 9, 2019
@guggero
Copy link
Collaborator

guggero commented Nov 21, 2020

Looks like this is checked by golangci-lint now. Can be closed?

@halseth
Copy link
Contributor Author

halseth commented Nov 24, 2020 •
edited
Loading

Looks like this is checked by golangci-lint now. Can be closed?

@guggero Looks like it is not totally checked by the linter. I rebased it and re-ran. One import change only though :)

@halseth halseth requested review from wpaulino and guggero December 17, 2020 14:19
@halseth halseth added this to the 0.13.0 milestone Dec 18, 2020
guggero
guggero approved these changes Dec 18, 2020
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🌮

@Roasbeef Roasbeef removed the blocked label Jan 28, 2021
@halseth halseth merged commit 10a81e0 into lightningnetwork:master Jan 29, 2021
@cfromknecht cfromknecht mentioned this pull request Feb 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfromknecht cfromknecht cfromknecht left review comments

@wpaulino wpaulino wpaulino approved these changes

@guggero guggero guggero approved these changes

Assignees
No one assigned
Labels
code health Related to code commenting, refactoring, and other non-behaviour improvements needs review PR needs review by regular contributors P3 might get fixed, nice to have
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@halseth @guggero @cfromknecht @wpaulino @Roasbeef