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

added convenience import #30

Merged
merged 6 commits into from Jan 24, 2021
Merged

added convenience import #30

merged 6 commits into from Jan 24, 2021

Conversation

maxsolomonhenry
Copy link
Collaborator

You can take this or leave it. I've been exploring a bit the python package structure.

in essence, this allows for the example.py import:

from ddspdrum.module import ADSR # etc...

to become:

from ddrpdrum import ADSR # etc...

@jorshi
Copy link
Collaborator

jorshi commented Jan 22, 2021

I'm happy with this change -- I think it makes sense given the scope of this project. The modules are the main components so it makes sense to have them accessible at the root package level. Just fix up the linter error and good from my perspective!

@maxsolomonhenry
Copy link
Collaborator Author

maxsolomonhenry commented Jan 22, 2021 via email

@maxsolomonhenry
Copy link
Collaborator Author

Ah, I see. It's saying

src/ddspdrum/__init__.py:1:1: F401 'ddspdrum.module.ADSR' imported but unused etc... in __init__.py but there's nothing to use in those modules for in the init file... So I'm not sure what to do here.

@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #30 (ed84a70) into main (7df9bee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   96.65%   96.66%           
=======================================
  Files           5        6    +1     
  Lines         449      450    +1     
=======================================
+ Hits          434      435    +1     
  Misses         15       15           
Flag Coverage Δ
pytest 45.54% <100.00%> (+0.17%) ⬆️
unittests 96.22% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
examples/examples.py 100.00% <100.00%> (ø)
src/ddspdrum/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7df9bee...ed84a70. Read the comment docs.

@turian
Copy link
Collaborator

turian commented Jan 23, 2021

@jorshi is the pythonic way that init.py just imports the submodules, rather than the things in it?

If not, @maxsolomonhenry I think the answer is just to add a pylint disable. But let's see what @jorshi says

@jorshi
Copy link
Collaborator

jorshi commented Jan 24, 2021

I've seen arguments for both having empty __init__ files as well as importing specific things from with submodules. Argument for having empty __init__s seems to be that they can become complex as the library grows. I think it makes sense here to import the main items (SynthModules / TorchModules) in the __init__ and keep the other stuff under their submodule namespace. i've seen that pattern in other libraries like librosa. So in that case we can add the pylint disable and move forward with this.

@maxsolomonhenry maxsolomonhenry merged commit fcc518c into main Jan 24, 2021
@maxsolomonhenry maxsolomonhenry deleted the module_refactor branch January 24, 2021 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants