WIP: First attempt to implement an interactive mode #325

FlorianWilhelm · 2020-08-23T14:49:54Z

@abravalheri: This is the first attempt to add an interactive mode to PyScaffold as described in #191. It was just way more complicated as I thought and took quite a while to only get a minimal version running. Please check it out if you find some time and give me your most honest feedback ;-) There is still a lot to do and I am not 100% sure if this is the way to go as the inquirer has quite some rough edges and combining command-line options with user prompts is quite difficult if the number of parameters depends on an unknown number of installed extensions.

That being said, a minimal version works and there are some ToDos in the source code. Let me know what you think. I am on another trip the next 3-4 days, so if you want to work on this branch, feel free to do so :-)

abravalheri

Wow, it is very impressive @FlorianWilhelm. Thank you very much for putting so much energy on this, and sorry for the time it took me to review it 😢

It is a real pity that the argparse lib is not made with extensibility in mind, and that it requires a huge amount of complexity to do anything... For example, I was wondering why we need to override the add_argument_group method, just to realize that argparse uses it behind the scenes to implement positional arguments (even if we don't call the method directly)... I can only imagine all the trouble you went to figure out those things. Very good job.

Some minor comments:

I feel like noprompt=True is a bit over a roundtrip in the brain, would it be better to have: prompt=False?
In the is_included maybe a way of making the function work is to check is the action.option_strings correspond to any of the ext.flags (since not always action.const will be available).
I suppose we can leverage the command value of the parsed opts to avoid conditionally checking for the interactive mode...

I was trying to analyse what are the causes of the complexity in the parser, and attempting to find ways of simplifying it... For me, it seems that the complexity boils down to the following points:

In order to cover argparse's API that are currently exposed to PyScaffold's users we need to overwrite add_argument_group, add_mutually_exclusive_group and add_argument
To keep track of the actions already processed, we need to overwrite the code that creates the actions, and also wrap them.

In the end, these 2 points lead to all the problems of dealing with mixins and having to re-implement some internal parts of argparse.

I wonder if we can avoid the mixins if we simply refuse to support some parts of the argparse API in interactive mode? More specifically: can we simplify the cli_parser file, if the only commit to support add_argument in the interactive mode? (I have already remove the mutually exclusive group between the django and cookiecutter extensions for v4 anyway, since I always find it a lacky abstraction... For pretend and list_actions we can just use the last one as winner, I don't think it would be a non-intuitive outcome...)

Another thought is that we might be able to also side step the need of tracking the actions that were already processed if we find a different way of checking it... What if we check the original arg list for the option_strings? Would it be problematic?

I am putting together a proof of concept for these ideas and I hope to submit a PR to your branch soon, so you can have a better understanding of what I am trying to say. Please feel free to use just the parts you like, or even ignore.

abravalheri · 2020-11-10T05:14:57Z

src/pyscaffold/cli_parser.py

+
+    An extension can include another using :obj:`~pyscaffold.extensions.include`
+
+    FIXME: This still doesn't work!


Maybe by checking action.options_string and ext.flag instead of trying to get action.const? (the later is not guaranteed to exist)

Right, I think checking ext.flag is way smarter :-)

abravalheri · 2020-11-10T05:17:03Z

src/pyscaffold/cli_parser.py

+        return False
+
+
+def merge_user_input(opts: ScaffoldOpts, input: Any, action: Action) -> ScaffoldOpts:


I suppose we need to convert the opts back to a namespace object, pipe it to the Action __call__ and then convert it back to a dict... Should we try to do this before converting the namespace object into a dictionary to avoid going back and forth?

I have always found the namespace object of argparse quite annoying as it creeps everywhere into your code if you are not careful and it feels not as natural as a dictionary, which is expected by many other libraries. So my first intuition would be to draw the line at the interface between argparse and Pyscaffold where the conversion should take place. So PyScaffold only deals with dictionaries everywhere and only when argparse functionality is called, it's converted to a namespace object. Or would you have an elegant solution to keep handling the namespace object to a minimum, so that it doesn't creep everywhere into the codebase?

Ok, that makes sense, and we don't have to be super efficient anyway...

I suppose merge_user_input, would be something along the lines(?) :

def merge_user_input(parser: ArgumentParser, opts: ScaffoldOpts, input: Any, action: Action) -> ScaffoldOpts: namespace = argparse.Namespace(**opts) action(parser, namespace, input, action.option_strings[-1]) # the last argument could also be omitted I suppose return vars(namespace)

The action should mutate the namespace as a side effect, so I suppose this will automatically merge the existing opts with any augmentation coming from the action...

abravalheri · 2020-11-10T05:19:05Z

src/pyscaffold/cli.py

@@ -169,7 +185,11 @@ def parse_args(args: List[str]) -> ScaffoldOpts:
        extension.augment_cli(parser)

    # Parse options and transform argparse Namespace object into common dict
-    return _process_opts(vars(parser.parse_args(args)))
+    opts = vars(parser.parse_args(args))
+    if opts["interactive"]:


I suppose we can avoid branching here by relying on the dynamic behaviour of dest="command" for the-i flag...

Smart, really smart!

abravalheri · 2020-11-10T05:19:48Z

src/pyscaffold/cli.py

+        "-V",
+        "--version",
+        action="version",
+        noprompt=True,


Would prompt=False be better than noprompt=True?

Absolutely, totally agree! What was I thinking? Normally I try to avoid double negations :-)

abravalheri · 2020-11-10T10:56:17Z

By the way, I really liked the way the interactive mode teaches the user how to use putup... By leaving the flag strings explicitly there, the more the users that are not familiar with the CLI use --interactive, the more they end up (passively) learning the parameters in the CLI, and more likely they are to start typing the arguments directly on the command line...

This reminds me a bit magit (one of my favourites interactive interfaces) and how involuntarily people discovers new git switches...

abravalheri · 2020-11-10T12:46:11Z

That being said, an alternative to interactive could be editable...

Imagine that instead of an CLI constantly querying the user, we spawn $EDITOR with an text file with contents similar to putup -h, where users can uncomment flags / review the parameters that they might have already typed directly as arguments. When the file is closed putup runs with it's contents (a kind of 2 passes system, first we preprocess things with cli.parse_args, then we compose the file based on [(a.option_strings, a.help) for a in parser._actions] and leverage argparse's fromfile_prefix_chars and convert_arg_line_to_args to parse the tempfile a second time?).

I wonder how these ergonomics/UX would compare to the interactive mode and how much we can reduce in terms of complexity (e.g. do we even need to overload ArgumentParser this way?)

FlorianWilhelm · 2020-12-20T21:16:07Z

src/pyscaffold/log.py

@@ -196,6 +197,10 @@ def __init__(
        self.formatter = formatter or ReportFormatter()
        super(ReportLogger, self).__init__(self._wrapped, self.extra)

+        level = environ.get("PYSCAFFOLD_LOG_LEVEL")


@abravalheri Should we document this environment variable PYSCAFFOLD_LOG_LEVEL somewhere or is it meant for development usage only?

I used this to debug some errors that I was seeing and decided to leave as it is, supposing it could be useful in the future (specially when debugging things that happen before the CLI processes the verbosity level)...

I suppose final users should not have a direct usage for it... We could include it in the log module documentation but I would prefer not to commit on this being part of the public API (just an internal API, for PyScaffold developers only).

Maybe we should revive this PYSCAFFOLD_LOG_LEVEL env var...

# Conflicts: # setup.cfg # src/pyscaffold/extensions/__init__.py

Merge v4.x into interactive

abravalheri · 2020-12-29T22:13:45Z

Sorry the PR got automatically closed when I merged the v4 branch on master and deleted the branch, but it is still valid 😅

I understand that the current idea is to make the "editable" the new "interactive", because it is less complex. Once I finish testing and checking for bus in the editable, if it is still less complex we can adopt that for v4. If it turns up to be as complex, we can try again the code in the interactive branch.

First try

f2129b1

FlorianWilhelm requested a review from abravalheri August 23, 2020 14:49

FlorianWilhelm added the enhancement New feature or request label Aug 23, 2020

FlorianWilhelm changed the title ~~First try for an interactive mode~~ First attempt to implement an interactive mode Aug 23, 2020

FlorianWilhelm changed the title ~~First attempt to implement an interactive mode~~ WIP: First attempt to implement an interactive mode Aug 23, 2020

Some code improvements

db358ae

abravalheri reviewed Nov 10, 2020

View reviewed changes

abravalheri mentioned this pull request Nov 10, 2020

Attempt to simplify complexity on #325 #331

Merged

abravalheri mentioned this pull request Nov 13, 2020

PoC/WIP: Extension to provide an "editable" interface to putup (in opposition to "interactive") #333

Closed

abravalheri added a commit that referenced this pull request Nov 27, 2020

Add nice fixes from #325

5404f39

abravalheri mentioned this pull request Nov 27, 2020

General fixes #335

Merged

abravalheri added a commit that referenced this pull request Nov 28, 2020

Add nice fixes from #325

28581e1

abravalheri added 4 commits December 20, 2020 22:01

Attempt to fix some tests for interactive

921a620

Attempt to simplify cli_parser

37c9cbc

Fix some bugs that appeared during simplification

690bcbf

Remove message that is no longer user from cli_parser

8084f9f

FlorianWilhelm commented Dec 20, 2020

View reviewed changes

FlorianWilhelm and others added 4 commits December 22, 2020 13:33

Fix comment

c04549c

Merge remote-tracking branch 'origin/v4.0.x' into interactive

0c03b6f

# Conflicts: # setup.cfg # src/pyscaffold/extensions/__init__.py

Added forgotten inquirer lib

62261d3

Merge pull request #350 from pyscaffold/merge_v4.x_into_interactive

6888e7b

Merge v4.x into interactive

abravalheri closed this Dec 29, 2020

abravalheri deleted the branch v4.0.x December 29, 2020 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: First attempt to implement an interactive mode #325

WIP: First attempt to implement an interactive mode #325

FlorianWilhelm commented Aug 23, 2020 •

edited

Loading

abravalheri left a comment

abravalheri Nov 10, 2020

FlorianWilhelm Dec 20, 2020 •

edited

Loading

abravalheri Nov 10, 2020

FlorianWilhelm Dec 20, 2020

abravalheri Dec 20, 2020 •

edited

Loading

abravalheri Nov 10, 2020

FlorianWilhelm Dec 20, 2020

abravalheri Nov 10, 2020

FlorianWilhelm Dec 20, 2020

abravalheri commented Nov 10, 2020

abravalheri commented Nov 10, 2020 •

edited

Loading

FlorianWilhelm Dec 20, 2020 •

edited

Loading

abravalheri Dec 20, 2020

abravalheri Jan 7, 2021

abravalheri commented Dec 29, 2020


		An extension can include another using :obj:`~pyscaffold.extensions.include`

		FIXME: This still doesn't work!

		return False


		def merge_user_input(opts: ScaffoldOpts, input: Any, action: Action) -> ScaffoldOpts:

WIP: First attempt to implement an interactive mode #325

WIP: First attempt to implement an interactive mode #325

Conversation

FlorianWilhelm commented Aug 23, 2020 • edited Loading

abravalheri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianWilhelm Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Nov 10, 2020

abravalheri commented Nov 10, 2020 • edited Loading

FlorianWilhelm Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abravalheri commented Dec 29, 2020

FlorianWilhelm commented Aug 23, 2020 •

edited

Loading

FlorianWilhelm Dec 20, 2020 •

edited

Loading

abravalheri Dec 20, 2020 •

edited

Loading

abravalheri commented Nov 10, 2020 •

edited

Loading

FlorianWilhelm Dec 20, 2020 •

edited

Loading