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!: Incorporate metadata classes into translators #225

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

Conversation

jarbesfeld
Copy link
Contributor

closes #224

Some points:

  • I'm still noticing a large number of warning when running pytest. Are these fine to ignore?
  • I changed the config sequence regex to allow only A,T,C,G or any non-numeric character. Some callers report non-numeric characters in the contig sequence to indicate features such as the breakpoint between the two transcripts.
  • Some of the translators allow a large number of parameters if they contain the metadata fields. Is if good practice for these functions to have a lot of specified parameters, or should I reformat?

@jarbesfeld jarbesfeld added enhancement New feature or request priority:high High priority stale-exempt labels Jan 15, 2025
@jarbesfeld jarbesfeld requested a review from korikuzma January 15, 2025 19:50
@jarbesfeld jarbesfeld self-assigned this Jan 15, 2025
@korikuzma
Copy link
Member

I'm still noticing a large number of warning when running pytest. Are these fine to ignore?

You can ignore the ones caused by fusor's dependencies, but should create a separate issue for resolving ones related to fusor directory (such as the event loop warning)

I changed the config sequence regex to allow only A,T,C,G or any non-numeric character. Some callers report non-numeric characters in the contig sequence to indicate features such as the breakpoint between the two transcripts.

This should be done in a separate PR IMO since its not directly related to #224 (keep commits clean + separate concerns).

Some of the translators allow a large number of parameters if they contain the metadata fields. Is if good practice for these functions to have a lot of specified parameters, or should I reformat?

I'd recommend creating a separate issue to address this. Consider creating Pydantic classes for fusion callers. https://docs.astral.sh/ruff/rules/too-many-arguments/#builtins has default 5 max args, but I think if you're using 10+ you should consider restructuring.

Base automatically changed from issue-222 to main January 16, 2025 13:53
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:high High priority stale-exempt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants