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

"Interactive mode" via editable file #362

Merged
merged 20 commits into from Jan 3, 2021

Conversation

abravalheri
Copy link
Collaborator

@abravalheri abravalheri commented Dec 29, 2020

#333 got accidentally closed, this is a continuation of that PR.

TODO list:

  • Test more thoroughly and try to find/fix bugs
  • Hide private argparse functions (parser._actions, parser._actions) in a centralised location so they can be easily changed if necessary
  • Change short help text to something similar to: "Interactively choose and configure PyScaffold's parameters"
  • Rename --edit to -i/--interactive

@abravalheri abravalheri force-pushed the editable branch 2 times, most recently from 53686d4 to f1cca87 Compare January 2, 2021 23:00
@abravalheri abravalheri marked this pull request as ready for review January 3, 2021 01:10
Copy link
Member

@FlorianWilhelm FlorianWilhelm left a comment

Choose a reason for hiding this comment

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

Really nice code! I really dig the implementation of the class Interactive. Tons of small functions are used like Lego to get the job done. That's clean code for me!

from zope import subpackage
# zope is the namespace and subpackage is the package name

To be honest, there is really only the `Zope project <http://www.zope.org/>`_
that comes to my mind which is using this exotic feature of Python's packaging system.
Chances are high, that you will never ever need a namespace package in your life.
Copy link
Member

Choose a reason for hiding this comment

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

Great comment! I think statements like that help users a lot in figuring out which features are important for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @FlorianWilhelm thanks for the comment, the original idea of mentioning Zope is yours 😝, I added the code example with the namespaces to make it more concrete...

Copy link
Member

Choose a reason for hiding this comment

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

But the sentence Chances are high, that you will never ever need a namespace package in your life. is from you right? That's the one I was referring to.

config_from_ext = getattr(ext, CONFIG_KEY, fallback_config)
return acc | set(config_from_ext.get(kind, []))

return reduce(_reducer, list_all_extensions(), initial_value)
Copy link
Member

Choose a reason for hiding this comment

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

General note: It always takes me several minutes to understand what's happening in a reduce function and I think I am not the only one as Guido von Rossum once stated something similar. At that point, I have no better idea and this functional style is really smart but makes it also hard to understand what's going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can revert it to a more procedural approach.

The main idea is to collect all the interactive configuration dicts from the extensions and merge them with CONFIG.

Copy link
Member

Choose a reason for hiding this comment

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

Or you add your last sentence as a comment just before the return statement to make it easier for future contributors to understand what's going on.

"""


@lru_cache(maxsize=2)
Copy link
Member

Choose a reason for hiding this comment

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

Does this speed things up a lot? Just asking as introducing caching can always lead to quite subtle bugs later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the idea is that I call this function for every action, and everytime it loads the extensions from the entrypoints (so Python goes to setuptools files collecting entries and then do some just-in-time file loading). I am not sure how long it takes, I added the cache there just intuitively to be sincere. I agree the best would be to measure before doing any premature optimisation 😅

To be fair, once all the extension classes and entrypoints are defined, this thing should be pretty much immutable (it is config metadata after all and works as a "distributed" constant across the extensions)... There could be problems if we are messing around with stubbing and mocking in the tests, but instead I am stubbing the get_config function itself instead of the things it depends on, so the cache is also taken away.


from .exceptions import ShellCommandException
from .log import logger

PathLike = Union[str, os.PathLike]
EDITORS = ("sensible-editor", "nvim", "vim", "nano", "subl", "code", "notepad", "vi")
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a comment here for the Sphinx module reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add that, thank you!

@abravalheri
Copy link
Collaborator Author

Thank you very much for the review @FlorianWilhelm, I conducted a series of tests and tried to have 100% coverage on this extension... Hopefully it should work fine now. Just in case I added some docs saying it is experimental and asking for feedback from devs.

Hopefully this fulfils the interactive needs of PyScaffold.

@FlorianWilhelm FlorianWilhelm added this to the v4.0 milestone Jan 3, 2021
@FlorianWilhelm FlorianWilhelm added the enhancement New feature or request label Jan 3, 2021
@abravalheri abravalheri merged commit b6b8c9e into pyscaffold:master Jan 3, 2021
@abravalheri abravalheri deleted the editable branch January 3, 2021 17:48
@FlorianWilhelm
Copy link
Member

Yes! 🥳 This was the last open todo for version 4.0, right @abravalheri? As your PR resolves issue #191.

@abravalheri
Copy link
Collaborator Author

abravalheri commented Jan 3, 2021

Precisely! Now I guess it is just a matter of checking if the docs are in good shape, and we could have a release 😄

I am happy in terms of features for v4. I think everything else can wait until v5 (even removing --no-pyproject), with the advantage that we can have some feedback (and wait until the adoption of the newest PEPs stabilizes/is supported by setuptools).

@FlorianWilhelm
Copy link
Member

So far I found the docs in quite good shape. Regarding --no-pyproject we could leave it for the time being as you suggested. So whenever you feel like it, I am happy to see a release version 4.0 :-) The honor of releasing it should be all yours as you did like 99% of all the work. I cannot thank you enough for taking so much good care of PyScaffold.

@abravalheri
Copy link
Collaborator Author

I will release a final beta before hand and wait a couple of weeks yet, just to be on the conservative side 😝 ...

Meanwhile I can have a look on the docs and also release updated versions of the extensions.

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

Successfully merging this pull request may close these issues.

None yet

2 participants