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

style: fix long lines using golines #453

Closed
wants to merge 1 commit into from

Conversation

bap2pecs
Copy link
Contributor

@bap2pecs bap2pecs commented Jul 1, 2024

Summary

long lines are hard to read and go fmt doesn't fix it for us. this PR is generated purely by running golines . -w to wrap lines at 120 length

unfortunately we can't enforce it in .golangci.yml by adding

linters:
  enable:
    - lll

due to golangci/golangci-lint#3983

but we can probably add a check like: https://github.com/babylonchain/finality-provider/pull/352/files

Test Plan

make lint

wait for CI

@SebastianElvis
Copy link
Member

Not sure if we should use multiple linters for the same project, as those linters might give comflicting suggestions. Let's see others' opinions as well.

Also, if the concept is ACK'ed, could we submit PR to the public dev branch rather than the base branch?

@bap2pecs
Copy link
Contributor Author

bap2pecs commented Jul 1, 2024

Also, if the concept is ACK'ed, could we submit PR to the public dev branch rather than the base branch?

yes I am only doing it as a RFC first :)

@maurolacy
Copy link
Contributor

This will add an extra pain to the rebasing / merging from the main dev branch process.
I don't think we should be doing this. At least, not at this point in the dev cycle.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

I propose we don't do this now. Not even against the main dev branch.

@bap2pecs
Copy link
Contributor Author

bap2pecs commented Jul 2, 2024

agree that it's better to hold onto it. after we consolidate the two branches would be a better timing

@maurolacy
Copy link
Contributor

Closing / delayed until base and dev are merged.

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.

3 participants