Skip to content

fix(position): PartialSourcePosition only support getCompilationUnit#1963

Merged
monperrus merged 2 commits intoINRIA:masterfrom
tdurieux:fixpartialsourceposition
Apr 15, 2018
Merged

fix(position): PartialSourcePosition only support getCompilationUnit#1963
monperrus merged 2 commits intoINRIA:masterfrom
tdurieux:fixpartialsourceposition

Conversation

@tdurieux
Copy link
Copy Markdown
Collaborator

No description provided.

@pvojtechovsky
Copy link
Copy Markdown
Collaborator

I fixed the code which asks for position and doesn't checks that there is partial source position only.

But I am not happy with that solution, so feel free to rollback it ... it would be nicer if there would be a better way how to detect whether position
A) is valid - SourcePositionImpl
B) not valid - either NoSourcePosition or PartialSourcePositionImpl

I can imagine several solutions:
S1) PartialSourcePositionImpl extends NoSourcePosition
S2) boolean SourcePosition#isValid() ... or any better name
S3) SourcePosition#getSourceStart returns -1 for invalid SourcePosition and >=0 for valid source position. Other methods of SourcePosition might behave same
S4) ?

@tdurieux @monperrus WDYT?

@spoon-bot
Copy link
Copy Markdown
Collaborator

API changes: 1 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180405.225214-41 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT

visibility reduced
Old method ElementPrinterHelper#createListPrinter(boolean, String, boolean, boolean, String, boolean, boolean, String)
New method ElementPrinterHelper#createListPrinter(boolean, String, boolean, boolean, String, boolean, boolean, String)
Breaking binary: breaking

@tdurieux
Copy link
Copy Markdown
Collaborator Author

tdurieux commented Apr 14, 2018

Thank you for the fix.

I think I prefer the solution S1 but I would prefer to throw an exception instead of -1 in NoSourcePosition.

@monperrus
Copy link
Copy Markdown
Collaborator

ready for merge?

@pvojtechovsky
Copy link
Copy Markdown
Collaborator

@monperrus please see problem and possible solutions above and give us your opinion please. Thanks ;-)

@pvojtechovsky
Copy link
Copy Markdown
Collaborator

  1. I like the idea of throwing of exception, because it avoids bugs caused by using invalid value -1.

what about change of NoSourcePosition and to throw exception there too?

  1. I would like to have an easy way how to detect that SourcePostion is correct.

S1) PartialSourcePositionImpl extends NoSourcePosition

validity detected by: position instanceof NoSourcePosition == false

  • it is longer then S2
  • client must know internal implementation class to detect that

S2) boolean SourcePosition#isValid() ... or any better name

validity detected by: position.isValid()

@pvojtechovsky
Copy link
Copy Markdown
Collaborator

This PR is a step in a good way. So it is ready for merge. We can do other thinkgs in another PRs.

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.

4 participants