Skip to content

Allow writing out custom annotation fields to cif files#669

Merged
padix-key merged 4 commits intobiotite-dev:mainfrom
Croydon-Brixton:feat/cif_write_custom_annotations
Oct 19, 2024
Merged

Allow writing out custom annotation fields to cif files#669
padix-key merged 4 commits intobiotite-dev:mainfrom
Croydon-Brixton:feat/cif_write_custom_annotations

Conversation

@Croydon-Brixton
Copy link
Copy Markdown
Contributor

feat: allow writing out custom annotation to cif file

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Oct 4, 2024

CodSpeed Performance Report

Merging #669 will not alter performance

Comparing Croydon-Brixton:feat/cif_write_custom_annotations (2fbadb0) with main (231eefe)

Summary

✅ 44 untouched benchmarks

@padix-key
Copy link
Copy Markdown
Member

Thanks for implementing the feature the feature! I would propose a slight change: Instead of a boolean flag to automatically include all custom annotations, I would propose an extra_fields parameter, where the included annotations are named, because

  • it is more consistent with the extra_fields parameter in get_structure() and
  • Explicit is better than implicit according to the Zen of Python.

@Croydon-Brixton
Copy link
Copy Markdown
Contributor Author

Great points, thank you for the feedback @padix-key -- I'll address those and update the PR

@Croydon-Brixton
Copy link
Copy Markdown
Contributor Author

✅ Alright @padix-key - I've changed the API to use the extra_fields parameter as you suggested and added a mini test for the reading & writing of custom annotation.

Copy link
Copy Markdown
Member

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

This looks good, I would only propose a small change, to make the test more clear.

@padix-key
Copy link
Copy Markdown
Member

I see, that you are not in CONTRIB.rst, yet. You are welcome to add yourself if you like.

@Croydon-Brixton Croydon-Brixton force-pushed the feat/cif_write_custom_annotations branch from 2fbadb0 to 0548106 Compare October 16, 2024 17:10
@Croydon-Brixton
Copy link
Copy Markdown
Contributor Author

Thank you -- it'd be an honour!
I've added myself & implemented your suggested change & rebased. Should be good to merge from my side, after CI passes -- you're welcome to squash the commits ofc

@padix-key padix-key merged commit cadb80a into biotite-dev:main Oct 19, 2024
@Croydon-Brixton Croydon-Brixton deleted the feat/cif_write_custom_annotations branch January 27, 2025 13:44
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