Skip to content

Move annotation start/end out of writers#89

Merged
LeoFeatherstone merged 4 commits intomainfrom
Wytamma-patch-4
May 30, 2025
Merged

Move annotation start/end out of writers#89
LeoFeatherstone merged 4 commits intomainfrom
Wytamma-patch-4

Conversation

@Wytamma
Copy link
Contributor

@Wytamma Wytamma commented May 30, 2025

I've moved the annotation start [ and end ] out of the writers so the writers won't break the newick spec.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes the annotation delimiters by moving the [ and ] wrappers out of the individual writer functions and into the newickRecurse function.

  • In newickRecurse, annotation output is now wrapped with [ and ]
  • beastAnnotation and nhxAnnotation no longer emit their own brackets
Comments suppressed due to low confidence (1)

src/io/writers/newick.ts:60

  • [nitpick] After refactoring the annotation delimiters, add tests for nodes with and without annotations to verify that empty annotations do not produce [] and that valid annotations remain correctly formatted.
if (node.branchLength !== undefined) {

@Wytamma Wytamma requested a review from Copilot May 30, 2025 07:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR moves the handling of annotation start/end markers out of individual annotation writers so that the writers no longer embed brackets and thereby break the spec.

  • The newickRecurse function now extracts the annotation string and conditionally wraps it with brackets.
  • The beastAnnotation and nhxAnnotation functions no longer prepend/append brackets, thus centralizing annotation formatting.
Comments suppressed due to low confidence (3)

src/io/writers/newick.ts:58

  • Confirm that wrapping the annotation output with '[' and ']' in newickRecurse aligns with the intended specification, now that the writers only return the raw annotation content.
res += `[${annotation}]`;

src/io/writers/newick.ts:83

  • Ensure documentation is updated to reflect that beastAnnotation no longer appends its own bracket wrappers, and now only returns the core annotation content for external wrapping.
res += '&';

src/io/writers/newick.ts:120

  • Update documentation/comments for nhxAnnotation to indicate that it no longer handles its own closing bracket, and that the external caller is now responsible for adding the proper annotation delimiters.
res += '&&NHX:';

Copy link
Contributor

@LeoFeatherstone LeoFeatherstone left a comment

Choose a reason for hiding this comment

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

Thanks @Wytamma this is a vastly better solution!

@LeoFeatherstone LeoFeatherstone merged commit e543bf2 into main May 30, 2025
2 checks passed
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