napari / cookiecutter-napari-plugin

Cookiecutter for napari plugins
BSD 3-Clause "New" or "Revised" License
67 stars 39 forks source link

pre-commit is breaking the package #121

Closed cnstt closed 2 years ago

cnstt commented 2 years ago

When running pre-commit, it deletes some imports in the init (of this package https://github.com/deep-finder/napari-deepfinder ) that are necessary for the testing to work properly:

from ._reader import napari_get_reader
from ._writer import write_annotations_xml
from ._widget import denoise_widget, reorder_widget, AddPointsWidget
from ._orthoview_widget import Orthoslice

Is this the wanted behaviour for pre-commit? Because maybe I using it the wrong way, but it is causing more damage than improvements for me currently ...

Czaki commented 2 years ago

this is a bug in cookiecutter fixed in #119 (not merged yet). This is problem of lack of __all__ in __init__

cnstt commented 2 years ago

Thanks a lot! Will there be a way to "update" a plugin afterwards with this change or is a manual intervention needed?

Czaki commented 2 years ago

Thanks a lot! Will there be a way to "update" a plugin afterwards with this change or is a manual intervention needed?

Based on my knowledge there is a need for manually fixing.

You need to add this line to __init__.py

__all__ = ("napari_get_reader", "write_annotations_xml", "denoise_widget", "reorder_widget", "AddPointsWidget", "Orthoslice")
tlambert03 commented 2 years ago

Note on updating, we don’t have this in our docs, but if you use cruft instead of cookiecutter when starting the project, you can use it’s update feature

https://github.com/cruft/cruft

cnstt commented 2 years ago

(I also have this issue with pre-commit https://github.com/pre-commit/pre-commit/issues/2411, but this is probably not related to #119)

cnstt commented 2 years ago

pre-commit is also changing package name from package-name to package_name, although I was fine with the current name...

tlambert03 commented 2 years ago

You can remove setup-cfg precommit if you’d like

cnstt commented 2 years ago

You can remove setup-cfg precommit if you’d like

For the moment I just totally removed setup-cfg as it was creating too much trouble for me ^^

tlambert03 commented 2 years ago

Yep; you should feel free to add/remove anything you want to tune it to your liking

Czaki commented 2 years ago

pre-commit is also changing package name from package-name to package_name, although I was fine with the current name...

First, it is strongly suggested to use an underscore in the package name https://peps.python.org/pep-0008/#package-and-module-names

At second, because of the naming convention of files in pypi warehouse the distributable files (sdist, wheel) will have underscore, nod dash in name, so using dash will introduce a little inconsistency.

cnstt commented 2 years ago

What led me to choose the package name with the dash is:

Czaki commented 2 years ago
  • the setup process of cookiecutter "napari-foobar"

Maybe we should fix this

tlambert03 commented 2 years ago

First, it is strongly suggested to use an underscore in the package name https://peps.python.org/pep-0008/#package-and-module-names

I disagree... from the pep you linked @Czaki:

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.

That actually doesn't state that a package named napari-foobar is incorrect. In fact, there's plenty of info to suggest that pypi itself will convert underscores to dashes for the package name. (In any case, when you pip install, it will work either way). So, in my opinion, setup-cfg format is wrong in this particular case.

tlambert03 commented 2 years ago

in fact, we actually prohibit underscores in package names during the cookiecutter process:

https://github.com/napari/cookiecutter-napari-plugin/blob/96891d3348d2744dbfd38fb0b140914597fec094/hooks/pre_gen_project.py#L16

This has been discussed before on zulip... so let's stick with our current behavior. if you want to disable setup-cfg format, that's fine