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

Cleaned up all the warn-unused and warn-unused-import issues #244

Closed

Conversation

norbert-radyk
Copy link
Contributor

@norbert-radyk norbert-radyk commented Apr 21, 2018

The change is a pure cleanup change (no functional changes) to fix the problems reported by the unused checks (-Ywarn-unused and -Ywarn-unused-import) and enforcing the checks by adding -Xfatal-warnings (will result in a compilation error should any new unused code section be introduced).

In some cases (where the method is a public one and can be overriden by the client) in order not to break the API removing the warning required marking the parameter explicitly as unused based on a solution proposed here: https://stackoverflow.com/questions/46128561/excluding-type-evidence-parameters-from-analysis-in-scala-when-using-ywarn-unus

…ling compilation error upon occuring unused problem.
def page(stats: Stat*) = """
def page(stats: Stat*) = {
stats.unused
"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe missing s interpolation 🤔 ?

function drawVisualization() {{
{Unparsed(funcCalls(stats))}
}}

          function drawVisualization() {{
-            {Unparsed(funcCalls(stats))}
+            ${Unparsed(funcCalls(stats))}
          }}


package object squeryl {

implicit class IdOps[A](a: A) {
Copy link
Member

Choose a reason for hiding this comment

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

I think "-Xfatal-warnings" and IdOps are too much 😕

@norbert-radyk
Copy link
Contributor Author

Hi @xuwei-k, I have addressed all the changes you requested by removing -Xfatal-warnings and IdOps and cleaning up the BartChartRenderer to use correct string interpolation.

I believe there would be a some value in having the fatal warnings being switched on, but you're probably right that the obfuscation it leads to (by the means of adding IdOps) is too much. So let's just treat this merge request as a cleanup of the unused code.

@xuwei-k
Copy link
Member

xuwei-k commented May 16, 2018

Hmmm 🤔 I think some (not private method) parameters (e.g. ev: CanCompare[T1, T2] ) has meaning. Don't remove these params.
Could you please separate more small pull requests? (e.g. "remove unused imports", "add return type annotations", "remove unnecessary comments")

@norbert-radyk
Copy link
Contributor Author

Thanks @xuwei-k for the feedback. Having separate merge requests for the smaller sets of changes is a fair point. I am going to close this one and have raised a small one for the fix on the BarChartRenderer which was failing with the unused import: #248

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

2 participants