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

__all__ generation? (and other things) #1

Open
vltr opened this issue May 25, 2018 · 6 comments
Open

__all__ generation? (and other things) #1

vltr opened this issue May 25, 2018 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@vltr
Copy link

vltr commented May 25, 2018

Hi! First of all, thanks for this nice tool! I think this tool is something rarely perceived but a very common task for Python programmers and it is super welcome!

Anyway, not that this is an issue, I assume that there are plans to implement this (for linter happiness), but I could also give some thoughts about the __all__ variable because not every single variable from a certain module is needed in the __init__.py file, like an import from another module. So, I can define what I want inside __all__ and that's what will shows up in the __init__.py file for that module, right?

Well, I want to share some situations ... I'm not here to say "hey, this tool is worthless because it doesn't check this or that", but rather give my use case so you can evolve it in the right direction 😉 I really appreciate your effort!

  1. Let's say I have a big file with a lot of classes (like a models.py file), and I don't want to export anything but my model classes, so I'll do something like this:
_all_locals = []

for k in dir():
    if k.startswith("_"):
        continue
    if k in locals() and inspect.isclass(locals().get(k)):
        if issubclass(locals().get(k), BaseModel):
            _all_locals.append(k)

__all__ = tuple(_all_locals)

Unfortunately, this doesn't work with static mkinit generation ... Mostly because of the AST, I presume? Given the module root, the AST shouldn't be able to retrive the __all__ variable result? In this case, it's a plain module.

  1. Filters: it is basically related to the above question. What if, inside __init__.py we could define some filters to what should or shouldn't belong to the __init__.py file? Example:
AUTOGEN_INIT_EXCLUDE = "lambda module, member: not member.startswith('_')"
AUTOGEN_INIT_INCLUDE = "..."
  1. Module level imports: I think this should be optional, I don't find very useful to have a __init__.py file like this (see comments in code):
# <AUTOGEN_INIT>
from my.app.models import core  # i don't need this
from my.app.models import distributed  # nor this
from my.app.models.core import (Base, BaseModel, metadata,)
from my.app.models.distributed import (Address, Contact, Device,)
# </AUTOGEN_INIT>
  1. Another nice option / feature would be to generate relative imports instead of the full module path, example:
AUTOGEN_INIT_RELATIVE_IMPORT = True
# <AUTOGEN_INIT>
from .core import (Base, BaseModel, metadata,)
from .distributed import (Address, Contact, Device,)
# </AUTOGEN_INIT>
  1. For last (but not least important), to make linters and / or code style checkers happy, I think mkinit could (optionally) create the __all__ variable at the end of the file / AUTOGEN_INIT block ... Assuming the above example:
AUTOGEN_INIT_RELATIVE_IMPORT = True
AUTOGEN_INIT_ALL = True
# <AUTOGEN_INIT>
from .core import (Base, BaseModel, metadata,)
from .distributed import (Address, Contact, Device,)

__all__ = (
    "Base",  # from .core
    "BaseModel",  # from .core
    "metadata",  # from .core
    "Address",  # from .distributed
    "Contact",  # from .distributed
    "Device",  # from .distributed
)
# </AUTOGEN_INIT>

Well, those are all ideas and possible proposals that would significantly boost productivity, at least in my use case. Again, this issue is just to give an use case and opinion on how mkinit could be a really awesome tool (to me). I'm not here to give destructive thoughts, since I already think this is a great tool and can evolve to something really, really slick in the future! 😄

My best regards and thanks for your work!!
Richard.

@vltr vltr changed the title __all__ generation? __all__ generation? (and other things) May 25, 2018
@Erotemic
Copy link
Owner

Hey @vltr, thanks for giving my tool a try! It's a huge motivator to see this kind of constructive feedback. Lets go point by point.

  1. Correct, your case won't work with static generation. There is no way to generally handle such cases using static analysis without simulating the code, and in that case why bother with static analysis?
    However, note that mkinit does have a dynamic backend, although it isn't fully hooked up yet. However, it wouldn't be out of the question to put a regular expression attribute name matcher that could be passed to the command line tool. Perhaps there could even be a __mkinit_export__ directive to explicitly state what should be exported. I'm planning on hooking up / documenting how to use the dynamic backend, but I'm not sure when I'll be able to get to it.

  2. I think the idea is good, but I'm not in love with the using comments to define code that will be executed as a directive. I think defining a regex and using a pattern matching solution might do the trick. It is going to take some thought to find the right implementation.

  3. Yes, there does need to be a mechanism for this preference. This should actually be pretty easy to do in the current architecture. If you want to try tackling this in a PR let me know, otherwise I'll get to it once I have some time.

  4. This feature is definitely on my want list. This should be another easy one implementation wise. Probably just a few line changes and passing down a flag variable. Again, PRs are welcome, but I do want this feature in a 0.1.0 release.

  5. Honestly, I didn't know that you could silence the linters by having an __all__ variable in your init file. It makes me happy to know I'll never need to write another # flake8: noqa again. It should be simple to add this feature by modifying mkinit.static_mkinit._initstr.

@Erotemic Erotemic added this to the 0.1.0 milestone May 26, 2018
@Erotemic Erotemic added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels May 26, 2018
@vltr
Copy link
Author

vltr commented May 26, 2018

I'm glad to help, @Erotemic !

Just like black, I think this tool can make a lot of sense in Python development. I'll go by each point as you did:

  1. I mentioned my "alternative" that could not be handled statically because it is something being done not only by me, you can also track some of this "tactics" on way bigger projects. I was just trying to understand the reach of your tool. Perhaps generating the __all__ variable of each file could be an extra feature of this tool? I mean, __all__ variables directly influence the result of the generated __init__.py file.
  2. I agree with you that running code from variables or inside comments may not be the greatest way, but you can keep in mind that this is something that sometimes is used as default behavior in Python itself 😄
  3. I would love to make a PR, but for now I have no time to even write the docs of one of the projects I help with, imagine new contributions ... But in two or three weeks I think I'll have some spare time, let's see!
  4. Same as above 😬
  5. Ha! I'm happy I could help a fellow Python developer to solve one of the boring tasks of the language! 😉

Anyway, one thing that came to mind now is that using AUTOGEN_INIT_ variables could raise some linter noises, which leads us to use # noqa on them ... Or use them inside comments, just like most linters works (including # noqa). I think it's just a case of finding a balance 💪

@Erotemic Erotemic removed good first issue Good for newcomers help wanted Extra attention is needed labels May 27, 2018
@Erotemic
Copy link
Owner

Erotemic commented May 27, 2018

I wasn't aware of black (I usually use autopep8). I'll check it out.

  1. I would like to eventually make it so you can ask mkinit to run dynamically when running it from the command line. Currently there exists a way to run dynamically from within python, but to autogenerate anything you need to use a command line flag (when running your program), which isn't very clean. Now that I've separated out the logic for output formatting in the static version, the next step for this point is to hook it into the dynamic version.

  2. Its funny that you link to timeit. I made my Timerit tool as an alternative exactly because timeit requires code represented as strings to run. I've been planning on separating it from ubelt and making it a standalone module from some time now, but writing this comment gave me the motivation to actually do it. 😄

  3. Understood, I think I actually handled points 3, 4, and 5, in Several Changes (options, refactoring, and bugfixes) #3

My earlier point was not about # NOQA on a single line, I'm ok with that. I'm much less ok with # flake8: noqa because it suppresses linting on an entire file.

For the remaining point 1 and 2, I'm going to make new issues. I'll leave this issue open as a milestone until the all points are satisfied.

@vltr
Copy link
Author

vltr commented May 28, 2018

Sorry for the delay! I was using autopep8 as well, but black provides such concise code formatting that's almost a pity not to use it. Of course, it's still on beta and you have to add one or two warnings into your flake8 ignore list, but for now that's okay. You can include it in your Makefile or git hook, well, it's an awesome tool. Just a tip: use the --safe flag 😄

  1. I think this could end up as a nice set of tools. I'm just wondering here this tool integrated with vscode (what I use today to code) and / or other IDEs / editors ... It will be simply awesome!
  2. Hahaha I'm glad to help! I think I'll even use it to rewrite one of my gists, which will serve as parameter for a project I have in mind and in need to implement;
  3. Yeah, I saw Several Changes (options, refactoring, and bugfixes) #3 yesterday! Great job, thanks a lot! 😉

I understood what you told about # flake8: noqa ... I was just figuring out if using variables inside __init__.py like AUTOGEN_INIT_ALL = True # noqa was a good idea (I mean, it was my suggestion (quite vague actually), so I just wanted to give something else to think about). For now, this looks really good! Thanks for your time, @Erotemic !

@Erotemic
Copy link
Owner

@vltr,

The newest 0.2.0 release begins to handle points 1 and 2. You can now specify variables in the __init__.py that modify the behavior of autogeneration.

While this doesn't implement pattern matching, you can use the __explicit__, __private__, and __protected__ variables to define lists of variable / module names. This is what they do:

  • explicit: includes all listed variables in the autogenerated __all__ variable. (useful when you have custom and auto-generated code).
  • private: will import the module name but non of its attributes
  • private: will not import any name that appears in this list.

If you think pattern matching is absolutely necessary, it may be reasonable to allow glob-style strings as items in these lists. I think if that feature lands, then all 5 points will be checked off correct?

Note, that the first feature can be handled by specifying an __all__ variable in the module where you only want to export very specific things. By default mkinit will respect a submodule's __all__ variable.

@vltr
Copy link
Author

vltr commented Feb 17, 2019

Hello @Erotemic ! Thanks a lot for the heads up, I had no time to add mkinit to any script or automatic process (yet), but I'm working on some tools that may lead me to do so (I hope).

If you think pattern matching is absolutely necessary, it may be reasonable to allow glob-style strings as items in these lists. I think if that feature lands, then all 5 points will be checked off correct?

Well, yes. I really don't mind (myself) about pattern matching, but I've seen countless Python source codes to know that this is in fact a requirement, but I think a glob matching would do almost all the job. Example glob matching patterns I have found in the wild (but I don't recall now where (exactly)):

  • _*: generally not available in __init__.py files, let's say, _Base, _Container, _Posix, etc;
  • *Models: to be available, like I mentioned earlier in one of the comments;
  • Abstract*, *ABC or *Enum to be available;
  • py* to not be available (like py3, py2, etc);

Other cases would require a more complex pattern matching, like any stdlib import, some internal flags (like DEBUG, even though it can be part of __private__) or any "full underscore" variable names, such as py3 or py2, that sometimes are used in the source code specially when you're dealing with the presence of one lib and, if not present, fallback to another, like this snippet from the Sanic framework 😉

But, as always, good work on mkinit, it is really an exceptional tool for any Python utility tool belt IMHO 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants