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

Initial invoke trait and auto-indent example #466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tailhook
Copy link
Contributor

@tailhook tailhook commented Dec 3, 2020

Here is a somewhat minimal implementation of invoking commands from validator trait.

General notes: it's unclear what to do with command::Status which can be Proceed or Submit. Currently it's ignored. But we may want to disable (i.e. assert on) commands returning Submit in validator and don't expose Status type to the user?

About auto-indent (#453), I've put mostly working example, but there are few ugly parts of the implementation:

  1. Data validated without newline. So to indent the new line, validator inserts line manually
  2. Which is a problem as any NewLine command triggers validation again (infinite recursion), so I protect for that in validator itself using RefCell, but this seems ugly. Should we disable validation for commands run from validator? Or in future anywhere except typed by keys?
  3. After inserting NewLine i need avoid it inserting again by the execute function. I use "empty message trick". But I'm not sure it's future-proof.

So while auto-indentation is just one use case for invoke, I think at least (3) should be fixed somehow. Quick options are:

  1. Insert newline before validation (probably breaks too much stuff)
  2. Add a validator option method, like validate_after_newline() -> bool { false } that makes it optional for validator. Or (2a) have a config option, or (2b) have different method for post-validation.
  3. Add some ValidationContext::skip_newline() method, to skip adding newline after validation

(note: (1) and (2) fix RefCell issue too, unlike (3))

@gwenn
Copy link
Collaborator

gwenn commented Jan 10, 2021

Which is a problem as any NewLine command triggers validation again

#293 (comment)
Currently, we have: Key -> Command -> Logic
If we do: Key -> Command -> Action(s) -> Logic and make sure invoke can only call Actions,
Then we should be fine.
However, this is a huge refactoring...

"All problems in computer science can be solved by another level of indirection" / "But it also can add complexity to the system, and therefore is not a free lunch."

@tailhook
Copy link
Contributor Author

I'm not sure i understand. Do you think that validation works on "Command" but invoke triggers "Action(s)"? This may be a problem too. What if actions changed text in a way that change the result of a validation?

Anyway, what next step(s) you anticipate? Do we make all this layer of indirection all at once?

@gwenn
Copy link
Collaborator

gwenn commented Jan 11, 2021

I'm not sure i understand. Do you think that validation works on "Command" but invoke triggers "Action(s)"? This may be a problem too. What if actions changed text in a way that change the result of a validation?

Even if your custom mapping/command triggers many actions, refresh/repaint and validation should be performed only once depending on the final result.

Anyway, what next step(s) you anticipate? Do we make all this layer of indirection all at once?

I am afraid we have no choice.
And if possible, you should also try to support all kind of filters.

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.

2 participants