Allow namespaces to be set to python module #2000
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Tarrasch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jcrobak, @steenzout and @freider to be potential reviewers. |
Tarrasch
force-pushed
the
allow_namespace_as_module_path
branch
from
1cbe481
to
9db6af3
Compare
|
Rendered docs screenshots:
|
|
Anyone wants to review? @erikbern @dlstadther |
|
lgtm but are namespaces widely used? idk |
dlstadther
previously requested changes
Feb 2, 2017
luigi/task.py
Outdated
| ``scope=__name__``. | ||
|
|
||
| The ``scope`` keyword makes so this call is only affective for task classes | ||
| with a matching [*]_ ``__module__``. The default value for ``scope`` is the |
There was a problem hiding this comment.
English correction (if understand correctly):
The scope keyword makes it so that this call is only effective for task classes with a matching [*]___module__.
luigi/task.py
Outdated
|
|
||
| .. code-block:: python | ||
|
|
||
| class Task2(luigi.Task): | ||
| task_namespace = 'namespace2' | ||
|
|
||
| This explicit setting takes priority over whatever is set in |
There was a problem hiding this comment.
over whatever is set in the
luigi/task.py
Outdated
| reasons: | ||
|
|
||
| * Two tasks with the same name will not have conflicting task families | ||
| * It's more pythonic, as modules are pythons recommended way to |
luigi/task.py
Outdated
| * Two tasks with the same name will not have conflicting task families | ||
| * It's more pythonic, as modules are pythons recommended way to | ||
| do namespacing. | ||
| * It's traceable, when you see the full name of a task, you can immedietly |
There was a problem hiding this comment.
It's traceable. When you see the full name of a task, you can immediately
luigi/task.py
Outdated
| * It's traceable, when you see the full name of a task, you can immedietly | ||
| identify where it is defined. | ||
|
|
||
| We recommend to call this function from your package's outermost |
There was a problem hiding this comment.
We recommend calling this function...
|
@erikbern, that's a really good concern actually. I'm not sure either if people use namespaces. But here's some related facts and thoughts:
@dlstadther, thanks. I'll fix these lingual issues right away. :) |
This have been a problem I've thought about for years. And also consciously gradually refactored the code base in previous patches towards making this relatively short path. Coming up with the final syntax and implementation also took me a while. Initially I imagined there would be a [config] parameter one could set, or specify by command line. But it turned out to be quite troublesome: * It's not clear how much it should affect. Should it also affect the things luigi exports? * It's hard to transition into it. Imagining a company with independent teams, the `auto_namespace` function can be applied package by package, luigi-version by luigi-version to whoever chooses to apply this new convention. This solution came out to be pretty clean, and with readable tests and detailed and a bit opinionated documentation.
Tarrasch
force-pushed
the
allow_namespace_as_module_path
branch
from
9db6af3
to
8550df4
Compare
|
sounds good! |
Tarrasch
dismissed
dlstadther’s stale review
I've adressed all the errors in the latest commit :) (also, this is first time im pressing dismiss)
kreczko
pushed a commit
to kreczko/luigi
that referenced
this pull request
Mar 28, 2017
This have been a problem I've thought about for years. And also consciously gradually refactored the code base in previous patches towards making this relatively short path. Coming up with the final syntax and implementation also took me a while. Initially I imagined there would be a [config] parameter one could set, or specify by command line. But it turned out to be quite troublesome: * It's not clear how much it should affect. Should it also affect the things luigi exports? * It's hard to transition into it. Imagining a company with independent teams, the `auto_namespace` function can be applied package by package, luigi-version by luigi-version to whoever chooses to apply this new convention. This solution came out to be pretty clean, and with readable tests and detailed and a bit opinionated documentation.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This have been a problem I've thought about for years. And also
consciously gradually refactored the code base in previous patches
towards making this relatively short path.
Coming up with the final syntax and implementation also took me a while.
Initially I imagined there would be a [config] parameter one could set,
or specify by command line. But it turned out to be quite troublesome:
things luigi exports?
teams, the
auto_namespacefunction can be applied package bypackage, luigi-version by luigi-version to whoever chooses to apply
this new convention.
This solution came out to be pretty clean, and with readable tests and
detailed and a bit opinionated documentation.
Have you tested this? If so, how?
I'm going for a week-long no computer vacation soon. After that I'll try running this in production.