You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
My 2 cents, first impressions after looking at the code
The way the files are organized is not very (python package) standard, and makes it difficult to navigate and understand their purpose. For instance, it's not obvious what is purpose of the codpy.pyd file when looking at the path and name and for what architecture it's compiled. I suspect that going forward you would like to release a package using PyPi a bit of cleanup is therefore required. No space in path names Time Series, using apps as a folder name might confuse people coming from web application, etc.
I would strongly suggest setting by default black and flake8 to enforce code style, it is common to even enforce it pre-commit.
Print statements are scattered here and there and something like print("scikit_tools loaded") is never good practice unless they are used in an example script. Same thing with if __name__ == "__main__": leaving this in files makes it hard to understand if they are library functions or example scripts.
Mehdi,
Thank you for these remarks. I am taking them into account. I'll drop
another version ASAP, I'll let you know.
Best regards
Le mer. 19 janv. 2022 à 09:18, MBounouar ***@***.***> a
écrit :
My 2 cents, first impressions after looking at the code
- The way the files are organized is not very (python package)
standard, and makes it difficult to navigate and understand their purpose.
For instance, it's not obvious what is purpose of the codpy.pyd file
when looking at the path and name and for what architecture it's compiled.
I suspect that going forward you would like to release a package using PyPi
a bit of cleanup is therefore required. No space in path names Time
Series, using apps as a folder name might confuse people coming from
web application, etc.
- I would strongly suggest setting by default black and flake8 to
enforce code style, it is common to even enforce it pre-commit.
- Include docstrings and Typing is also something to encourage, the
later is not a must but because the code base is still small, I would say
it's very good practice and helps tremendously going forward.
- Print statements are scattered here and there and is something like print("scikit_tools
loaded") is never good practice unless they are used in an example
script. Same thing with if __name__ == "__main__": leaving this in
files makes it hard to understand if they are library functions or example
scripts.
- Testing (pytest) and using github workflows is a must and will
prevent a lot of troubles.
—
Reply to this email directly, view it on GitHub
<#1>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNCGWE7AA64LM3LDLIKRETUWZXT3ANCNFSM5MJHW5JA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
My 2 cents, first impressions after looking at the code
codpy.pyd
file when looking at the path and name and for what architecture it's compiled. I suspect that going forward you would like to release a package using PyPi a bit of cleanup is therefore required. No space in path namesTime Series
, usingapps
as a folder name might confuse people coming from web application, etc.black
andflake8
to enforce code style, it is common to even enforce it pre-commit.print("scikit_tools loaded")
is never good practice unless they are used in an example script. Same thing withif __name__ == "__main__":
leaving this in files makes it hard to understand if they are library functions or example scripts.pytest
) and using github workflows is a must and will prevent a lot of troubles. #2The text was updated successfully, but these errors were encountered: