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

refactor Transcriber class #144

Closed
NickleDave opened this issue Mar 26, 2022 · 1 comment
Closed

refactor Transcriber class #144

NickleDave opened this issue Mar 26, 2022 · 1 comment
Assignees
Labels
CLN: clean/refactor code clean-up: PEP8 fixes, refactors
Projects

Comments

@NickleDave
Copy link
Collaborator

Transcriber class is due for a re-factor; should be done in next version

  • clean up Transcriber __init__

    • I think currently it's possible to pass in a Meta instance as the argument for config--there's a check in the pre-conditions--but then nothing happens with this, it would just break
    • just drop all this functionality, no-one is using it. Also way too much logic in the __init__ in general
    • could add classmethods if we really want these options, e.g. 'Transcriber.from_config`
    • probably we should just remove the Meta anyway
  • more broadly what does Transcriber do?

    • provide a convenient way to do from_file and to_annot for all annotation formats,
      without needing to work with the functions (and soon, classes) directly
    • the "right" way to do this would be to pass in the functions themselves as arguments,
      and have a classmethod Transcriber.from_format('simple-seq') (I don't love the method name from_format)
class Transcriber:
    def __init__(from_file, to_annot):
         self.from_file = from_file
         self.to_annot = to_annot

    @classmethod
    def from_format(format):
        ...

    @classmethod
    def from_config(config):
        ...
@NickleDave NickleDave added the CLN: clean/refactor code clean-up: PEP8 fixes, refactors label Mar 26, 2022
@NickleDave NickleDave self-assigned this Mar 26, 2022
This was referenced Mar 26, 2022
NickleDave added a commit that referenced this issue May 1, 2022
@NickleDave NickleDave added this to In progress in ENH May 1, 2022
NickleDave added a commit that referenced this issue May 1, 2022
NickleDave added a commit that referenced this issue May 1, 2022
NickleDave added a commit that referenced this issue May 4, 2022
NickleDave added a commit that referenced this issue May 4, 2022
NickleDave added a commit that referenced this issue May 5, 2022
@NickleDave NickleDave moved this from In progress to Done in ENH May 7, 2022
@NickleDave
Copy link
Collaborator Author

  • the "right" way to do this would be to pass in the functions themselves as arguments,
    and have a classmethod Transcriber.from_format('simple-seq') (I don't love the method name from_format)

ended up doing basically this.
Transcriber.from_format('simple-seq'), for example, returns an instance of a SimpleSeq, already instantiated, using its from_file method. Methods like to_annot can then be called on this returned instance, instead of Transcriber having to handle them or even know they exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLN: clean/refactor code clean-up: PEP8 fixes, refactors
Projects
ENH
Done
Development

No branches or pull requests

1 participant