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

Add type annotations to nf-core/tools #2200

Closed
3 tasks done
mashehu opened this issue Mar 14, 2023 · 10 comments · Fixed by #2223 or #2483
Closed
3 tasks done

Add type annotations to nf-core/tools #2200

mashehu opened this issue Mar 14, 2023 · 10 comments · Fixed by #2223 or #2483

Comments

@mashehu
Copy link
Contributor

mashehu commented Mar 14, 2023

Description of feature

Typed Python will improve the development experience (and hopefully the code quality).

Intro to types in python: https://fastapi.tiangolo.com/python-types/

Tools we should add:

To lower the work load we will do it file by file (if possible)

Active PRs

@NovakApis
Copy link
Contributor

NovakApis commented Mar 27, 2023

quick question, do I provide types for variable declarations as well? For example in the nf-core/tools/nf_core/components/components_command.py at line 60 we have:

    local_component_dir = Path(self.dir, self.component_type, "local")
    # should I convert it to 
    local_component_dir : Path = Path(self.dir, self.component_type, "local")
    # ?
    # If so, I will try to do it for every declaration

@mashehu
Copy link
Contributor Author

mashehu commented Mar 27, 2023

yeah, looks good to me.

@mashehu
Copy link
Contributor Author

mashehu commented Mar 27, 2023

Since this is a bigger task, we probably should only convert one file at a time, to keep track of when something breaks

@fabianegli
Copy link
Contributor

Type (almost) annotations are always good to have. I find the most useful places to have them in functions and methods that are used by others - be it in the codebase or users of a module or package.

On a side-note: please make sure to use Python 3.7 to check the type annotations locally, if you do, because we still officially support it. Otherwise it will be easy to use newer capabilities of Python that won't work in all cases.

@mirpedrol
Copy link
Member

are used by others

To add an example, this would be a function that is used in an nf-core command.

@MatthiasZepper
Copy link
Member

FYI: I am currently doing a large rewrite of the nf-core download command that will also change some types. So maybe spare this file for now to not get into a large merge conflict?

I am happy to add the types to that one later in a separate PR, since I anyway now have a quite good understanding of the implementation.

@fabianegli fabianegli changed the title Convert nf-core/tools to typed python Add type annotations to nf-core/tools Mar 31, 2023
@kedhammar kedhammar assigned kedhammar and unassigned NovakApis Oct 16, 2023
@kedhammar kedhammar linked a pull request Oct 17, 2023 that will close this issue
1 task
@kedhammar
Copy link
Contributor

First PR (#2223) merged.

@kedhammar
Copy link
Contributor

PRs #2214 and #2218 ready for review

@kedhammar
Copy link
Contributor

Merged #2214 and #2218

@kedhammar
Copy link
Contributor

Submitted #2483 to implement PyUpgrade in pre-commit

@mashehu mashehu closed this as completed Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for review
6 participants