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

warning for unused symbols and stdout/stderr output validation #147

Merged
merged 8 commits into from
Mar 29, 2017
Merged

warning for unused symbols and stdout/stderr output validation #147

merged 8 commits into from
Mar 29, 2017

Conversation

gerdreiss
Copy link
Contributor

Additional PR for #104 for stdout/stderr output validation

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 69.304% when pulling 039c289 on gerdreiss:i104-symbol-not-used-stdout-test into a260f9a on hrj:master.

Copy link
Owner

@hrj hrj left a comment

Choose a reason for hiding this comment

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

Easy but important change required.

.foreach(d => {
println(s"${YELLOW}${BOLD}symbol '${d.name}' defined in ${d.pos.filename} line: ${d.pos.pos.line} but never used${RESET}")
})
}
Copy link
Owner

Choose a reason for hiding this comment

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

You have to make a recursive check for includedScopes as well. Test-case:

{
def y = 10
}

Note the braces which introduces a new scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but I think the method definitions in class Scope retrieves the parent and recursively the definitions from the included scopes in private def allLocalDefinitions:

private val localDefinitions = Helper.filterByType[Definition](entries)
private def allLocalDefinitions: Seq[Definition] = localDefinitions ++ includedScopes.flatMap(_.allLocalDefinitions)

  def definitions:Seq[Definition] = {
    val parentDefinitions = parentOpt.map(_.definitions).getOrElse(Nil)
    val allLocalDefs = allLocalDefinitions
    val allLocalNames = allLocalDefs.map(_.name)
    parentDefinitions.filter(d => !allLocalNames.contains(d.name)) ++ allLocalDefs
  }

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm.. you seem to be right. The test case fails though (no warning).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's curious... I'll check it tonight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the problem is that those nested scopes we're looking for are not within the includedScopes, but childScopes. The method checkUnusedSymbols() must be called on the childScopes, too. I'm going to push my change in a moment.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 69.39% when pulling 98724fc on gerdreiss:i104-symbol-not-used-stdout-test into a260f9a on hrj:master.

@hrj hrj merged commit 99dc6c2 into hrj:master Mar 29, 2017
@gerdreiss gerdreiss deleted the i104-symbol-not-used-stdout-test branch March 29, 2017 06:20
@hrj hrj added this to the 0.5.0 milestone May 10, 2017
@hrj hrj changed the title added stdout/stderr output validation warning for unused symbols and stdout/stderr output validation May 10, 2017
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