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

feat: make methods in ManeTranscript public #326

Merged
merged 1 commit into from
Jul 23, 2024
Merged

feat: make methods in ManeTranscript public #326

merged 1 commit into from
Jul 23, 2024

Conversation

korikuzma
Copy link
Member

close #229

  • ManeTranscript._validate_index -> ManeTranscript.validate_index
  • ManeTranscript._get_reading_frame -> ManeTranscript.get_reading_frame

close #229

* `ManeTranscript._validate_index` -> `ManeTranscript.validate_index`
* `ManeTranscript._get_reading_frame` -> `ManeTranscript.get_reading_frame`
@korikuzma korikuzma added enhancement New feature or request priority:low Low priority labels Jul 23, 2024
@korikuzma korikuzma requested a review from jsstevenson July 23, 2024 16:08
@korikuzma korikuzma self-assigned this Jul 23, 2024
@jsstevenson
Copy link
Member

👍 up to you if you want to do this here or save it for a more consequential case, but we can also keep the old methods for a time, have them re-direct to the newly-named methods, and use the warnings module to issue deprecation warnings until they're changed

technically, I also think you could argue this isn't a breaking change (because the previous thing was private and therefore not supposed to be accessed) but since Variation Normalizer was calling the private methods it'll still break compatbility. could go either way imo

@korikuzma
Copy link
Member Author

@jsstevenson

👍 up to you if you want to do this here or save it for a more consequential case, but we can also keep the old methods for a time, have them re-direct to the newly-named methods, and use the warnings module to issue deprecation warnings until they're changed

I'm pretty sure we're the only ones using Cool-Seq-Tool atm. I think this would be good to do in the future once we've made a major release. But if you think it's important to do now, I can.

technically, I also think you could argue this isn't a breaking change (because the previous thing was private and therefore not supposed to be accessed) but since Variation Normalizer was calling the private methods it'll still break compatbility. could go either way imo

I always go back and forth on these. I think this is a case where it would be nice to have documentation for our lab to decide what to do. I can update the title to have it not be a breaking change.

@korikuzma korikuzma changed the title feat!: make methods in ManeTranscript public feat: make methods in ManeTranscript public Jul 23, 2024
@jsstevenson
Copy link
Member

yeah fine either way imo

@korikuzma
Copy link
Member Author

@jsstevenson Okay, I'll just keep it as is.

@korikuzma korikuzma merged commit 1710fb0 into main Jul 23, 2024
14 checks passed
@korikuzma korikuzma deleted the issue-229 branch July 23, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:low Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark MANETranscript._validate_index() and MANETranscript._get_reading_frame() as public
2 participants