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

Using upstream goconst #1500

Merged
merged 5 commits into from Nov 17, 2020
Merged

Using upstream goconst #1500

merged 5 commits into from Nov 17, 2020

Conversation

iwankgb
Copy link
Contributor

@iwankgb iwankgb commented Nov 8, 2020

Thanks to jgautheron/goconst#11 we should be able to get rid of goconst fork. Closes #1495.

@iwankgb iwankgb marked this pull request as draft November 8, 2020 19:19
@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 8, 2020

jgautheron/goconst#13 must be merged and new version released before we proceed.

@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 8, 2020

@golangci/team, it seems that problem is more complicated here. Some time ago (our fork was based on 3-year old commit) new feature was introduced to goconst. It cause goconst to notice repeated strings in function calls. With current settings in .golangcilint.yml there are 50 places that would have to be fixed. When min-length and min-occurrences are set to 3 (default values) there is only 11 of them.

Taking above into consideration we have 2 options, I think:

  1. Fix violations and publish clear warning to the users stating that update in goconst may force them to do additional work.
  2. Reach out to @jgautheron and try to make checks configurable, with same default config in our project.

Second option sounds much more reasonable to me and I would file all the necessary PRs.

Third option (that I consider a non-option) would be to keep maintaining our fork and deviate from the upstream. This is probably the worst possible idea. In case of no feedback I will proceed with option 2 (if @jgautheron is open to such a solution).

@ernado
Copy link
Member

ernado commented Nov 8, 2020

Can we use default excludes to mitigate (1) or error messages are same in both cases?

@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 9, 2020

@ernado it should be possible, I will investigate.

@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 9, 2020

@ernado message is the same for all types of violation and I can't really tell one from another.

@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 13, 2020

I have file a PR against upstream goconst repository. If it's merged we should be able to start using upstream seamlessly.

@iwankgb iwankgb marked this pull request as ready for review November 17, 2020 16:22
@iwankgb
Copy link
Contributor Author

iwankgb commented Nov 17, 2020

@ernado, a quick look would be appreciated :)

Copy link
Member

@ernado ernado left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@iwankgb iwankgb merged commit 993337b into master Nov 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the upstream_goconst branch November 17, 2020 19:07
@ldez ldez added the linter: update version Update version of linter label Dec 7, 2020
@ldez ldez changed the title Using upstrem goconst Using upstream goconst Dec 28, 2020
@ldez ldez mentioned this pull request Feb 13, 2021
3 tasks
@ldez ldez added this to the v1.33 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: update version Update version of linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More configurable goconst
4 participants