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

Public dependency manager #297

Closed
wants to merge 16 commits into from

Conversation

moltob
Copy link
Contributor

@moltob moltob commented May 12, 2019

In order to implement #288, this PR stores the dependency manager in a global registry class before task are loaded or executed.

This allows to get it from any code inside a task generator, e.g. from clean to read the previous execution result, see the contained sample.

The original discussion suggested to implement the registry as plain Python dict. I'd like to prevent string-based access and therefore implemented a very simply static class in new module doit.globals.

@coveralls
Copy link

coveralls commented May 12, 2019

Coverage Status

Coverage decreased (-57.4%) to 41.667% when pulling 32b4307 on moltob:feature/public-dependency-manager into 2b59b89 on pydoit:master.

@moltob
Copy link
Contributor Author

moltob commented May 12, 2019

@schettino72 Where would I link a sample snippet into the docs, what would be a good spot?

@schettino72
Copy link
Member

I will rebase this before reviewing it...

@schettino72
Copy link
Member

Actually quite complicated to rebase this to master... I gave up.
I think it is easier no manually apply your changes. sorry for the trouble. I could do that myself later, not today.

Your Globals as Registry implementation is ok. I think the get_dep_manager turned out be useless. We can just ask user to import Globals and remove get_dep_manager. What do you think?

Regarding the documentation, your example i would like to put in a new HOW-TO section. dont worry about that now, we can do it later.

But Globals would need to be documented now, not sure where... here I would you use a simpler example (with just actions without clean).

@moltob
Copy link
Contributor Author

moltob commented May 12, 2019

Don't worry, I'll fix the branch and will incorporate your comments.

@moltob
Copy link
Contributor Author

moltob commented May 12, 2019

@schettino72 The diff is clean now, you can add your comments for the files, but I can easily just remove the access function as well. I'll do that tomorrow.

@moltob
Copy link
Contributor Author

moltob commented May 13, 2019

I just rebased the code cleanly onto master locally, will push it tonight.

Regarding the sample I'd like to keep the clean. IMHO this is one of the main use cases for that feature. A slightly simplified sample will be part of the pushed branch.

@moltob moltob force-pushed the feature/public-dependency-manager branch from 232c228 to ce47f50 Compare May 13, 2019 13:48
@moltob moltob changed the title (WIP) Public dependency manager Public dependency manager May 13, 2019
@moltob moltob force-pushed the feature/public-dependency-manager branch from b497297 to e22babe Compare May 13, 2019 14:00
@moltob moltob force-pushed the feature/public-dependency-manager branch from e22babe to 0cca250 Compare May 13, 2019 14:03
@moltob
Copy link
Contributor Author

moltob commented May 13, 2019

@schettino72 Please pick up review, rebase onto master is complete. Please see my above comments on having clean in the sample.

doit/globals.py Outdated
class Globals:
"""Accessors to doit singletons.

:cvar dep_manager: The doit dependency manager, holding all persistent task data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the type here

Copy link
Member

@schettino72 schettino72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it is still missing docs, right?
I guess this feature does not fit any of the existing pages... I would create a new page between task_args and uptodate.

Example is better now. There are so many use-cases for this feature...

I noticed sometimes users tend ignore a feature if the example is not trivial and do not reflect their use-case.
So I usually prefer minimal examples where the only feature being show is the one discussed. And leave to the user to figure out how put all doit features together to solve their problems :)

tests/test_cmd_base.py Show resolved Hide resolved
tests/test_cmd_base.py Outdated Show resolved Hide resolved
@moltob
Copy link
Contributor Author

moltob commented May 19, 2019

@schettino72 Docs added.

doc/globals.rst Outdated Show resolved Hide resolved
doc/globals.rst Outdated
@@ -0,0 +1,31 @@
==========
Singletons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the file is called 'globals' the the title is 'singletons'... we should use just one name...

you write the docs as if we would have many globals, not sure that would be the case. maybe we could expose the Reporter or some other internal stuff. But those are already documented elsewhere...

what about have this section of docs being specific to 'db backend' or something like that.

Another thing i just realized is that we are calling it 'dep_manager' as it is original function on doit itself.Wondering if we should use a more generic name. In practice users could use this to store some data. like a a task that stores and use a data itself in future runs. for this use-case the name dep_manager is completely innapropriate.

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll adapt the section title to match file and module name.

you write the docs as if we would have many globals

Correct, this way, it will be easy to extend it when needed. I certainly would not add anything here just to have more than one attribute.

Regarding the names: I surely stumbled upon the names initially as well. There is a class Dependency that is called dep_manager wherever it is used and in fact it is actually the doit database...

But I got over it 😃 If you want to rename it and clean this up, I'd ask you to do this in a separate PR, if you don't mind. I maybe would have called things differently here and there, but that is really a personal thing...

doc/globals.rst Show resolved Hide resolved
doc/globals.rst Show resolved Hide resolved
@moltob
Copy link
Contributor Author

moltob commented May 26, 2019

Added docs about dep_manager API, some of the backend API and the different backends.

@schettino72
Copy link
Member

Merged, thanks. Nice to have a regular contributor 👍

Notes:

  • There is a spell checker task for docs doit spell (requires installation of hunspell).
  • on docs I like breaking lines on period, etc. Makes it easier to review changes and not having very long lines (still max 80 column per line)
  • your sample was broken. there is also a task to check them doit samples_check (not sure they work on Windows).
  • after all I guess the name dep_manager is ok.
  • I also exposed Globals as doit.Globals, doit.globals.Globals was looking like Java 😛

@schettino72 schettino72 closed this Jun 2, 2019
@moltob moltob deleted the feature/public-dependency-manager branch June 2, 2019 09:09
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