Skip to content
This repository was archived by the owner on Sep 3, 2020. It is now read-only.

T1002281 class cast ex - #67

Open
kiritsuku wants to merge 2 commits into
scala-ide:masterfrom
kiritsuku:t1002281-class-cast-ex
Open

T1002281 class cast ex#67
kiritsuku wants to merge 2 commits into
scala-ide:masterfrom
kiritsuku:t1002281-class-cast-ex

Conversation

@kiritsuku

Copy link
Copy Markdown
Member

Instead of doing an unsafe cast, an exception is thrown directly (or a conversion if possible).

Second commit contains cleanups.

Review on Reviewable

Add public return types, add override keywords, remove trailing
whitespace
@ghprb-bot

Copy link
Copy Markdown

Test PASSed.

See Console Output in the link below for an update site containing this PR binary artefacts.

Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/42/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIUC, you really need a RangePosition there. Why not throw as well on OffsetPosition, rather than giving a RangePosition which has high chances of being false ( I expect callers will want the word at point) ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, and I wonder why we get an OffsetPosition in the first place...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess SI-8807 is one reason...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What could go wrong if OffsetPosition is treated as RangePosition? This way we need to ensure that no OffsetPosition is passed into scala-refactoring otherwise it blows up. Maybe that is the right thing to do, then I just need to fix the ticket on the IDE side.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There just shouldn't be a reason to use an OffsetPosition. Remember, they're only used for compiler generated trees, so if a refactoring is started from the editor, there should always be a better suited RangePosition available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup, and having an exception on OffsetPositions works as a canary.

@ghprb-bot

Copy link
Copy Markdown

Test FAILed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants