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

PDEX: Add friendlier error message for HidingEnclosingType #4451

Merged
merged 2 commits into from May 9, 2016

Conversation

Projects
None yet
3 participants
@rzats
Contributor

rzats commented May 8, 2016

Fixes #4228; is a duplicate of #4337 adjusted for some API changes.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 8, 2016

Contributor

Hi @rzats, thanks for your pull request.

Could you please remove the changes made in Problem and the args[0].equals(problem.getClassName()) in Simplifier? It is not necessary as it will always return true.

Contributor

JakubValtar commented May 8, 2016

Hi @rzats, thanks for your pull request.

Could you please remove the changes made in Problem and the args[0].equals(problem.getClassName()) in Simplifier? It is not necessary as it will always return true.

@rzats

This comment has been minimized.

Show comment
Hide comment
@rzats

rzats May 8, 2016

Contributor

@JakubValtar: here's why these are necessary - #4337 (comment)
Although I could remove the changes if this tweak is not required.

Contributor

rzats commented May 8, 2016

@JakubValtar: here's why these are necessary - #4337 (comment)
Although I could remove the changes if this tweak is not required.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 8, 2016

Contributor

However, you are not checking it for the name of the sketch... you are merely comparing problem.iProblem.getArguments()[0] with itself (which is the name of the problematic class).

class C {
  class C { // Error: The class C cannot have the same name as the name of your sketch
  }
}

If you want to compare with the sketch name, you would have to pass it into getSimplifiedErrorMessage. I would suggest changing the error message to something like 'The class "%s" cannot have the same name as your sketch or enclosing class', because the sketch name is currently not available in the Simplifier.

The simplifier will be changed soon to have more information about the sketch, so we will be able support these messages better.

Contributor

JakubValtar commented May 8, 2016

However, you are not checking it for the name of the sketch... you are merely comparing problem.iProblem.getArguments()[0] with itself (which is the name of the problematic class).

class C {
  class C { // Error: The class C cannot have the same name as the name of your sketch
  }
}

If you want to compare with the sketch name, you would have to pass it into getSimplifiedErrorMessage. I would suggest changing the error message to something like 'The class "%s" cannot have the same name as your sketch or enclosing class', because the sketch name is currently not available in the Simplifier.

The simplifier will be changed soon to have more information about the sketch, so we will be able support these messages better.

@rzats

This comment has been minimized.

Show comment
Hide comment
@rzats

rzats May 8, 2016

Contributor

@JakubValtar: thanks - this makes sense 😉 Updated accordingly.

Contributor

rzats commented May 8, 2016

@JakubValtar: thanks - this makes sense 😉 Updated accordingly.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 8, 2016

Contributor

@rzats No, thank you for your contribution!

@benfry Let's merge this and add TODO to check for sketch name when I'll be refactoring this.

Contributor

JakubValtar commented May 8, 2016

@rzats No, thank you for your contribution!

@benfry Let's merge this and add TODO to check for sketch name when I'll be refactoring this.

@benfry benfry merged commit 7bc3af8 into processing:master May 9, 2016

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

Great, thanks a lot folks.

Member

benfry commented May 9, 2016

Great, thanks a lot folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment