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

fix: removed file by mistake #3287

Merged
merged 3 commits into from
Dec 14, 2022
Merged

fix: removed file by mistake #3287

merged 3 commits into from
Dec 14, 2022

Conversation

tbruyelle
Copy link
Contributor

This file was removed by mistake in this PR that was merged w/o reviews :/

#2961

As a remind this file helps to track external tools version in the scaffolded chain.

@tbruyelle tbruyelle added the skip-changelog Don't check changelog for new entries label Dec 13, 2022
aljo242
aljo242 previously approved these changes Dec 13, 2022
View reviewed changes
@aljo242
Copy link
Contributor

aljo242 commented Dec 13, 2022

Can we actually change the path to .../files/tools/tools.go?

@tbruyelle
Copy link
Contributor Author

Can we actually change the path to .../files/tools/tools.go?

yep, done!

aljo242
aljo242 approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@aljo242 aljo242 left a comment

Choose a reason for hiding this comment

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

Works great for me! thanks

@aljo242
Copy link
Contributor

aljo242 commented Dec 13, 2022

What do y'all think about adding our linting tools and gofumpt into these dependencies?

Then creating some command like ignite check or ignite lint that wraps calling them?

Too opinionated perhaps? Seems like these tools are pretty widely used though.

@tbruyelle
Copy link
Contributor Author

tbruyelle commented Dec 13, 2022 •
edited
Loading

Too opinionated perhaps?

For gofumpt yes, we shouldn't get involved in code format, there's already gofmt, adding more than that is only personal taste. We should let the developer chose.

For the linter, that's a different question because it can help in finding bugs, like when an error is not checked or other things. Considering the importance of the code quality in a blockchain, and the irremediable damages caused by a potential bug, that makes sense to embed/wrap a linter in CLI. Good idea!

@aljo242
Copy link
Contributor

aljo242 commented Dec 14, 2022

#3288

@aljo242 aljo242 merged commit be5dae2 into main Dec 14, 2022
@aljo242 aljo242 deleted the fix/removed-file branch December 14, 2022 14:45
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
@tbruyelle @jeronimoalbi
fix: removed file by mistake (ignite#3287)
a9aff55 
Co-authored-by: Jerónimo Albi <[email protected]>
@vutting4221 vutting4221 mentioned this pull request Jan 2, 2024
Open
@vutting4221 vutting4221 mentioned this pull request Mar 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljo242 aljo242 aljo242 approved these changes

@jeronimoalbi jeronimoalbi jeronimoalbi approved these changes

@lumtis lumtis Awaiting requested review from lumtis

@fadeev fadeev Awaiting requested review from fadeev

@Pantani Pantani Awaiting requested review from Pantani Pantani is a code owner

Assignees
No one assigned
Labels
skip-changelog Don't check changelog for new entries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tbruyelle @aljo242 @jeronimoalbi