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

Add sbt-tpolecat to prevent common bug patterns #110

Merged
merged 5 commits into from
Apr 28, 2024

Conversation

froth
Copy link
Contributor

@froth froth commented Apr 17, 2024

See also: typelevel/cats-effect#4054

also update sbt to run with newer JVMs
(sbt/sbt#7235)

@TonioGela
Copy link
Member

Hello @froth, thanks for your contribution.
What a timing! I had created this branch here yesterday evening that fixes many issues in the build, sbt version, GitHub workflow and most lib versions.
There are overlaps between our branches, but I'm more prone to open a PR from mine to fix more things at the same time.
What I can offer you, if you like the idea is to rework this PR to add this scalacOption but via the addition of sbt-tpolecat to the build, WDYT?

@froth
Copy link
Contributor Author

froth commented Apr 17, 2024

@TonioGela timing indeed :) no worries, I can redo this change onto your code once merged.

I am not sure about sbt-tpolecat for this template however. I am a huge fan of the plugin and use it myself. However in the default configuration it treats warnings as errors. This could lead to frustration for people just wanting to try out cats-effect. They would have to remove all unused imports before trying out their code etc. What do you think?

We could add ThisBuild / tpolecatDefaultOptionsMode := DevMode, but I am not sure whether that sets a good example.

@TonioGela
Copy link
Member

@TonioGela timing indeed :) no worries, I can redo this change onto your branch once merged.

Thanks!

I am not sure about sbt-tpolecat for this template however. I am a huge fan of the plugin and use it myself. However in the default configuration it treats warnings as errors. This could lead to frustration for people just wanting to try out cats effect. They would have to remove all unused imports before trying out their code etc. What do you think?

Good point. I still prefer to add sbt-tpolecat as it adds a lot of sane defaults, so what we can do is add it and do one of the following:

  • Manually add ScalacOptions.fatalWarnings to tpolecatExcludeOptions
  • Setup as the default mode the verbose one, which helps developers to debug with more verbose warnings. The downside is that they will have to remember to set the environment variable SBT_TPOLECAT_CI on their CIs in order to enable fatal warnings again, but this can probably addressed with an inline comment just before setting the default mode in the build, right?

@froth
Copy link
Contributor Author

froth commented Apr 17, 2024

I think the underlying question is: what is the intended audience of this template. People wanting to try ce out or people bootstrapping production code(with CI, etc). As always the answer is "both", but I personally lean more towards the "trying out" usecase (my personal experience is that in $WORK related projects most people build their own templates anyways). Therefore I think that verbose mode and comment is a good compromise.

@TonioGela
Copy link
Member

but I personally lean more towards the "trying out" usecase

For this case there's scala-cli and the typelevel/toolkit :trollface:

Therefore I think that verbose mode and comment is a good compromise.

Okay, let's follow this path momentarily. I will try to ask more people what they think about this possibly new default before merging, as it may be really opinionated.

Once the PR is ready (this one or a new one that you may want to open), we can share the link in Discord and discuss it a bit before making any decision.

@froth
Copy link
Contributor Author

froth commented Apr 17, 2024

Once the PR is ready (this one or a new one that you may want to open), we can share the link in Discord and discuss it a bit before making any decision.

That sounds like a good solution.
I will reset this one to preserve our discussion. And I will have an eye on discord :)

- And fix some warnings
@froth
Copy link
Contributor Author

froth commented Apr 17, 2024

@TonioGela updated the PR

src/main/g8/build.sbt Outdated Show resolved Hide resolved
froth and others added 2 commits April 17, 2024 10:18
Co-authored-by: Antonio Gelameris <toniogela89@gmail.com>
@BalmungSan
Copy link

My two cents, just add sbt-tpolecat, it makes the management of all those flags pretty easy and Scala version resistant. Also, in general, most of the flags are useful for writing good CE apps.
If people find the warnings too cumbersome then they may tweak them. Adding info about the plugin being used and how to manage it, or a link to the plugin docs would be great.

Note however that, I am biased since I always use the plugin and I have never ever bothered to use the "development" mode.

@TonioGela
Copy link
Member

So, a lot of opinions have been expressed on Discord, and it seems that the leading opinion is prompting the user whether to add fatal-warnings or not with sbt-tpolecat always included.
@froth do you think you can rework the PR to add this flag maybe?

@froth
Copy link
Contributor Author

froth commented Apr 19, 2024

@TonioGela can do so. I think g8 wants a default for the option. I would use "without fatal" if that is ok

@TonioGela
Copy link
Member

@TonioGela can do so. I think g8 wants a default for the option. I would use "without fatal" if that is ok

I think the "vox populi" is in favor of keeping them enabled :)

@froth
Copy link
Contributor Author

froth commented Apr 19, 2024

@TonioGela can do so. I think g8 wants a default for the option. I would use "without fatal" as default, ok?

@BalmungSan
Copy link

Yeah it seems folks would prefer without fatal for newcomers.

Also, does the template generate a README? If so, it would be great to add a note about the plugin there.

@TonioGela
Copy link
Member

Yeah it seems folks would prefer without fatal for newcomers.

Also, does the template generate a README? If so, it would be great to add a note about the plugin there.

Yes, let's add a README and a note, lgtm

@froth
Copy link
Contributor Author

froth commented Apr 21, 2024

Thanks for the feedback :)

I added a paragraph to the README and made fatal warnings configurabel.

@froth froth changed the title Add -Wnonunit-statement to prevent common bug patterns Add sbt-tpolecat to prevent common bug patterns Apr 22, 2024
@TonioGela TonioGela merged commit 9eb09d3 into typelevel:main Apr 28, 2024
2 checks passed
@froth froth deleted the non-unit-statement branch April 28, 2024 11:50
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.

None yet

3 participants