Moving some libtaskotron code into modules to make the deptree lighter and open the door to execution on not-just-fedora
unit tests don't work, will fix if the concept appears sound
Details
Details
ran task-rpmlint, it works.
Diff Detail
Diff Detail
- Repository
- rLTRN libtaskotron
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Overall, I'm really liking this. Just a few questions, that would not even make me nack the diff.
| libtaskotron/directives/bodhi_comment_directive.py | ||
|---|---|---|
| 113–118 | While I get what you did there, It feels kind of unnecessary. "Standard" traceback gives the same information Traceback (most recent call last):
File "runtask.py", line 7, in <module>
from libtaskotron import runner
File "/home/src/taskbot_hub/libtaskotron/libtaskotron/runner.py", line 25, in <module>
from libtaskotron.directives import exitcode_directive
File "/home/src/taskbot_hub/libtaskotron/libtaskotron/directives/exitcode_directive.py", line 9, in <module>
from libtaskotron import check
ImportError: cannot import name checkjust in the form of ImportError instead of TaskotronExtensionError. Is having the custom exception message (and class) really worth the trouble here? | |
| libtaskotron/ext/__init__.py | ||
| 1 | I could use a comment explaining what this means. | |
| setup.py | ||
| 44 | ? | |
Comment Actions
I definitely prefer this to D581 :) Looks very simple and reasonable. I provided some suggestions for minor changes, but the overall approach looks good.
| libtaskotron.spec | ||
|---|---|---|
| 59 | I guess some distributions like OpenSUSE could be interested in having this in i.e. libtaskotron-rpm, it might be useful for them as well. The same applies for things like rpm_utils.py (at least partly). If we want to do this only when asked and keep it simple in the meantime, I'm OK with it. | |
| 74 | I'd prefer moving python-paramiko here as well, so that we save another dep (tree) when installing libtaskotron on a disposable client. We would probably want to rename this package to something like libtaskotron-remote. Alternatively, we can create something like libtaskotron-core or libtaskotron-minimal, which is going to get installed on the disposable client, and libtaskotron package would be meant for task developers - it would contain additional deps like python-paramiko, and it could even require libtaskotron-disposable, so that dnf install libtaskotron would get you the full package. It could even incorporate it (so that we don't have so many submodules), so we would have just libtaskotron-core and libtaskotron (and then libtasktron-OS packages). | |
| 150–152 | I guess listing all disposable modules here so that we don't have to do import libtaskotron.ext.disposable.foo is out of question? :) | |
| libtaskotron/directives/bodhi_comment_directive.py | ||
| 113–118 | I think we need a custom exception class here, inheriting from TaskotronError, so that people using libtaskotron as a library can easily guard against taskotron-related errors (even import errors) and it doesn't crash their own code. However, I'd personally call the exception TaskotronImportError (ExtensionError sounds too generic) and I'd put a function somewhere which will generate the error message instead of copy-pasting it into every try-import place. The function could explain that taskotron is broken up into multiple packages and you need to install additional modules if you want to use this directive. This way the user experience is definitely better than when we crash with just a generic ImportError - that really looks like a bug, not like something the user is responsible for. I wonder, could we avoid using try-import approach and just capture all ImportErrors in the directive loader code? It would decide whether the import error is related to one of our modules or not (using the libtaskotron. prefix) and if it was, it would print the user friendly message and wrap the error into TaskotronImportError and re-raise, otherwise simply re-raised the original. The handling would be in a central place and less work for us (no need to try-import stuff). Thoughts? | |
| libtaskotron/exceptions.py | ||
| 43–44 | If we explain what "extensions" are, I guess this is also a good name. But it seems a bit higher level than simple import error. Should it cover other issues as well (do you have some use cases in mind)? Maybe there's place for both TaskotronExtensionError and TaskotronImportError? | |
| libtaskotron/ext/__init__.py | ||
| 1 | +1. No idea here. | |
| libtaskotron.spec | ||
|---|---|---|
| 59 | yeah, I figure we'll end up doing that eventually for CentOS support etc. but I also think that we have enough other fedora-isms to extract from the code that this could be handled later | |
| 150–152 | I don't understand the connection between listing all the modules in the specfile and import statements, can you elaborate a bit more? | |
| libtaskotron/directives/bodhi_comment_directive.py | ||
| 113–118 | I like the idea of making sure that the "you need to install stuff" comes through clearly instead of just the exception. I'll see what I can do with the directive loading code | |
| libtaskotron/exceptions.py | ||
| 43–44 | Yeah, this could probably use a different name. not sure we really need TaskotronImportError but either way, TaskotronExtensionError is a little non-descript for what it's being used for here | |
| setup.py | ||
| 44 | PoC code - if I leave that line in place, the package won't build since there are test failures. this should never actually be merged | |
| libtaskotron/directives/bodhi_comment_directive.py | ||
|---|---|---|
| 113–118 | I understand, but wouldn't it make sense to add the "you need to install stuff" to the error message? This IMHO does not really give more information than the plain "could not import" error message. | |
| libtaskotron/exceptions.py | ||
| 43–44 | +1 on TaskotronImportError | |
| setup.py | ||
| 44 | /me was just interested, thx for the explanation. | |
| libtaskotron.spec | ||
|---|---|---|
| 150–152 | Sorry, I haven't explained it correctly. The idea was that we could keep all disposable-related modules directly in libtaskotron/, so that we could continue using import libtaskotron.vm instead of import libtaskotron.ext.disposable.vm. But it would require us to specify all disposable-related .py files in the libtaskotron-disposable part of the spec file (and I assume also specify the complementary files in the main libtaskotron section). But the pain in manually maintaining this doesn't seem to justify the shorter import lines. | |
Comment Actions
- adding comment to setup.py emphasizing temporary-ness of commented out line
- simplifying import error detection, addressing other concerns from review
Comment Actions
Looks good!
| libtaskotron/exceptions.py | ||
|---|---|---|
| 45 | Maybe the docstring could use a closer explanation. | |
Comment Actions
- fixing test failures introduced by module movement
- adding tests to cover ImportError changing
- re-enabling tests in setup.py
- small packaging fixes, dodo.py fix
I guess some distributions like OpenSUSE could be interested in having this in i.e. libtaskotron-rpm, it might be useful for them as well. The same applies for things like rpm_utils.py (at least partly). If we want to do this only when asked and keep it simple in the meantime, I'm OK with it.