Skip to content

Conversation

@ventusfortis
Copy link
Contributor

Replace unsafe head with headOption.orNull in CfgNodeMethods.method for annotations, so annotations not inside a method no longer throw NoSuchElementException, as described in #5699

Copy link
Contributor

@maltek maltek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

You deleted a new line there, could you please fix that?

@ml86
Copy link
Contributor

ml86 commented Jan 12, 2026

method is not supposed to return null. This does not solve the underlying problem of the broke assumption in the code. Can you please provide test input to illustrate the problem?

@maltek
Copy link
Contributor

maltek commented Jan 16, 2026

@ml86 I agree that randomly returning null is not how one is supposed to write scala code. Unfortunately, we already have existing code in that file that does the same for the same DSL method. I don't want to put up a higher bar on a new contributor's code than the existing implementation. So I would accept this. We can always convert the .method DSL to a proper Option based API even after merging this.

@ml86
Copy link
Contributor

ml86 commented Jan 16, 2026

@maltek No, just because there is already bad code in there, it does not mean that we will accept more. This kind of code will make our lifes hell in the future.

@ventusfortis ventusfortis requested a review from maltek January 19, 2026 07:31
@maltek
Copy link
Contributor

maltek commented Jan 19, 2026

@ventusfortis we've had some discussions about this topic outside here. We're not going to merge this PR here, but we'll solve the problem you had in different ways:

@ventusfortis
Copy link
Contributor Author

Ok, I agree, now I see issue.
I wanted to propose another solution to this with having #5762 in mind, but now I'll look into #5761. Thanks for answering!

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