Skip to content

Conversation

@Istar-Eldritch
Copy link
Contributor

No description provided.

@Istar-Eldritch Istar-Eldritch marked this pull request as ready for review December 9, 2025 14:21
line,
GFF3Attributes.TRANSL_EXCEPT,
feature.getAttributeByName(GFF3Attributes.TRANSL_EXCEPT));
feature.getAttributeByName(GFF3Attributes.TRANSL_EXCEPT).orElse(null));
Copy link
Contributor

@lognath-ebi lognath-ebi Dec 10, 2025

Choose a reason for hiding this comment

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

If we set this to null, it might throw an NullPointerException if not handled. For example, line 69 will definitely crash unless we handle it. So we’d have to be careful and add checks in all the places that use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. This reflects the existing behavior, I purposefully did not refactor the existing usage of the getAttributeByName function, just make the output compatible by unwrapping the Optional. Going forward we should not use nulls and instead leverage the Optional.

}
boolean isCircular =
Boolean.TRUE.toString().equalsIgnoreCase(feature.getAttributeByName(GFF3Attributes.CIRCULAR_RNA));
boolean isCircular = Boolean.TRUE
Copy link
Contributor

@lognath-ebi lognath-ebi Dec 10, 2025

Choose a reason for hiding this comment

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

While checking this line, I noticed that in most other places we explicitly use .orElse(null). Can this be refactored ?

  • We could introduce a getAttributeByNameOrNull method that returns a String, and handle the null checks directly since we’re already doing that in many places. Boolean handling can be kept separate if needed.
  • Or we can keep the existing Optional return type and continue handling it with .get(), isPresent(), or map() depending on the case.

Just a thought, open to whichever feels is cleaner.

Copy link
Contributor Author

@Istar-Eldritch Istar-Eldritch Dec 11, 2025

Choose a reason for hiding this comment

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

well, the reason I've done the orElse(null) is to not have to refactor the existing logic. Ideally, we should leverage the Optional instead of using nulls going forward. So your second point. If I were to do those changes the work would be even bigger and I didn't want to open that can of worms.

Copy link
Contributor

@Rajkumar-D Rajkumar-D left a comment

Choose a reason for hiding this comment

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

A thought about the attribute structure after reviewing this PR. Most attributes in a GFF3 feature are simple key–value pairs, but a few keys can contain multiple values. I propose the following approach:

  • Keep GFF3Feature.attributeMap private and store attributes internally as a
    private Map<String, List<String>> attribute
  • This keeps the underlying data model consistent and avoids type ambiguity.
  • Provide helper methods in GFF3Feature to manage attribute access:
  1. setAttribute(key, value) - sets or replaces a single attribute value
  2. addAttribute(key, value) - appends a value for multi-valued attributes
  3. getAttribute(key) - returns the first value for single-valued attributes
  4. getAttributeList(key) - returns the full list for multi-valued attributes

This keeps the implementation type-safe, hides the internal representation, and gives us flexibility to support both single-value and multi-value attributes.

So this will be the code change

geneName.ifPresent(v -> gff3Feature.addAttribute("gene", v));
id.ifPresent(v -> gff3Feature.addAttribute("ID", v));
parentId.ifPresent(v -> gff3Feature.addAttribute("Parent", v));

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