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

Feature/forward final #69

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Feature/forward final #69

wants to merge 20 commits into from

Conversation

mjunkin
Copy link
Collaborator

@mjunkin mjunkin commented Oct 8, 2024

Changes resulting from end-to-end testing of VDYP 7.9.

Copy link

sonarcloud bot commented Oct 9, 2024

// functions would make it less comprehensible
}
)
@SuppressWarnings("java:S3776")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The warning identifiers are rather opaque without comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe throwing an exception that explains the problem more clearly than "No value present" would be a good idea? The stacktrace helps but in cases where only the message makes it through it could stand to be more helpful. Not sure if it would be worth the effort though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it make more sense to put the CV output in a subclass instead of having it activated by setting an Optional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A static variable controlling matchers for all unit tests seems like a good way to end up with some really annoying test order dependant failures.

// BGRPFIND (Breakage uses decay BEC)
var breakageGroup = breakageGroupMap.get(genus, bec.getDecayBec().getAlias());
try {
// DGRPFIND
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it make more sense to have these methods on the resolved control map return an optional instead of having them throw an exception you catch here?

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.

2 participants