ASFHyP3 / hyp3-cookiecutter

Cookiecutter to quickly generate a new HyP3 Plugin
BSD 3-Clause "New" or "Revised" License
4 stars 5 forks source link

Plugin creation issue collection #26

Closed forrestfwilliams closed 1 year ago

forrestfwilliams commented 1 year ago

This is an issue where I'm recording issues I ran into while using the version of the cookiecutter currently in update-staging. I'll expand this into more actionable issues if necessary.

forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
git init .
git remote add origin https://github.com/ASFHyP3/hyp3-isce2.git
git add .
git commit -m "Minimal HyP3 plugin created with the hyp3 plugin cookiecutter"
git branch -M main
git push -u origin main

git tag -a v0.0.0 -m "Marking zeroth release for auto-versioning and CI/CD Tooling"
git push --tags

git checkout -b develop
git push -u origin develop
forrestfwilliams commented 1 year ago
Screen Shot 2023-03-23 at 3 11 51 PM
forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
Screen Shot 2023-03-23 at 3 16 22 PM Screen Shot 2023-03-23 at 3 16 33 PM
forrestfwilliams commented 1 year ago
Screen Shot 2023-03-23 at 3 19 38 PM
forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
forrestfwilliams commented 1 year ago
name: Checklist comment

on:
  pull_request:
    types:
      - opened
    branches:
      - main

jobs:
  call-release-workflow:
    uses: ASFHyP3/actions/.github/workflows/reusable-release-checklist-comment.yml@v0.7.1
    permissions:
      pull-requests: write
    secrets:
      USER_TOKEN: ${{ secrets.GITHUB_TOKEN }}
jhkennedy commented 1 year ago

In the hyp3-isce2 plugin, the process_isce2() function isn't available.

In __main__ we import hyp3_isce2: https://github.com/ASFHyP3/hyp3-cookiecutter/blob/update_staging/%7B%7Bcookiecutter.__project_name%7D%7D/src/%7B%7Bcookiecutter.__package_name%7D%7D/__main__.py#L11

but, the process module, nor the process_{{cookiecutter.__process_name}} function are in the __init__.py's __all__ statement: https://github.com/ASFHyP3/hyp3-cookiecutter/blob/update_staging/%7B%7Bcookiecutter.__project_name%7D%7D/src/%7B%7Bcookiecutter.__package_name%7D%7D/__init__.py#L7-L9

so they aren't in the package API and would have to be imported like from {{cookiecutter.__package_name}}.process import {{cookiecutter.__process_name}}

jtherrmann commented 1 year ago

@jhkennedy I believe we would only have to put from {{cookiecutter.__package_name}}.process import {{cookiecutter.__process_name}} in __init__.py to make the function part of the package API, but we would not have to put it in __all__.py, which is only used to specify what should be imported for from <module> import * statements, which in my understanding are not recommended anyway. Do you think we should support import * statements?

Also see https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

Edit: Ah, I'm now realizing that you probably did not mean that we should add that function to the package API, but rather fix the import statement in __main__. I implemented the former solution before I realized it was the wrong way to do it, see https://github.com/ASFHyP3/hyp3-cookiecutter/pull/32

jhkennedy commented 1 year ago

@jtherrmann the __all__ variable defines the public names in a package -- that is, it specifies the public API.

Yes, when you from thing import *, you import all the public names from the thing namespace into your namespace. Notably, if you don't define __all__, it still imports all names in thing not prefixed with an underscore or double-underscore as they are considered public by default. __all__ just restricts the public namespace.

Importantly, PyCharm and other tooling also inspect and respect the public namespace and will warn you if a name isn't discoverable in that namespace. E..g, the exceptions submodule in the the HyP3 SDK, which is not in hyp3_sdk.__init__.py (same behavior if you import it there, but don't put it in __all__): image

Really good explanation here: https://stackoverflow.com/a/35710527

jhkennedy commented 1 year ago

Also, PEP8, which we follow, recommends using __all__: https://peps.python.org/pep-0008/#public-and-internal-interfaces

jhkennedy commented 1 year ago

Edit: Ah, I'm now realizing that you probably did not mean that we should add that function to the package API, but rather fix the import statement in main. I implemented the former solution before I realized it was the wrong way to do it, see https://github.com/ASFHyP3/hyp3-cookiecutter/pull/32

Yes, I mostly meant we should fix the import statement. Separately, we might want to consider putting process in the package API though.

jtherrmann commented 1 year ago

@jhkennedy That all makes sense, thanks! I'll close this issue as fixed by https://github.com/ASFHyP3/hyp3-cookiecutter/pull/33