Conversation
|
I started a wiki page for Can you point me on how to make the community aware so they can also test? |
|
May I add these entries from my |
|
Hi Pedro, couple of high-level things first:
|
pslacerda1
force-pushed
the
new_command
branch
2 times, most recently
from
3f63232 to
a259980
Compare
And I don't know why the tests are failing, in my machine they're ok. 
|
|
Feel free to rebase and we'll look into test failures after. |
|
I simply rebased and forced with lease. The same inconsistency is happening, no error in my local. |
|
How are you running the tests? like in the CI, I can reproduce the issue locally as well with this branch. |
|
Maybe a context manager could improve these temporary settings (or specifically error handling)... |
|
This seems a little hacky. What is the issue here? Why can't exception be properly caught? |
|
I think the current behavior is ok because the test must fail early in case of errors so don't abuse servers too much. However it is failing too early giving me no chance to test exceptions. As the errors occurs inside a My best opinion is to create a context manager that temporarily set the option than restores: with cmd.invocation.options(exit_on_error=0):
cmd.run('fun my_foo=a')
assert 'MyError' in out+err |
|
But my problem is that the following code don't shows up even if called before exited (as required by the CI) 475 exc_type, exc_value, tb = colorprinting.print_exc(
476 [__file__, SCRIPT_TOPLEVEL]) |
|
Not sure if I entirely follow.
Seems like |
|
Because is not an error from
The An aspect I don't understand is why |
|
The exception is thrown from the parsing logic in diff --git a/modules/pymol/commanding.py b/modules/pymol/commanding.py
index e783682b9..6cac59aa9 100644
--- a/modules/pymol/commanding.py
+++ b/modules/pymol/commanding.py
@@ -749,18 +749,22 @@ SEE ALSO
# special _self argument
kwargs.pop("_self", None)
new_kwargs = {}
- for var, param in sign.parameters.items():
- if var in kwargs:
- value = kwargs[var]
- # special 'quiet' argument
- if var == 'quiet' and isinstance(value, int):
- new_kwargs[var] = bool(value)
+ try:
+ for var, param in sign.parameters.items():
+ if var in kwargs:
+ value = kwargs[var]
+ # special 'quiet' argument
+ if var == 'quiet' and isinstance(value, int):
+ new_kwargs[var] = bool(value)
+ else:
+ actual_type = resolved_hints.get(var, param.annotation)
+ new_kwargs[var] = _into_types(var, actual_type, value)
else:
- actual_type = resolved_hints.get(var, param.annotation)
- new_kwargs[var] = _into_types(var, actual_type, value)
- else:
- if param.default is sign.empty:
- raise RuntimeError(f"Unknow variable '{var}'.")
+ if param.default is sign.empty:
+ raise RuntimeError(f"Unknow variable '{var}'.")
+ except ArgumentParsingError as e:
+ print(e)
+ return |
|
Oh, I see the misconception. The error is raised inside new_command (or inner, _into_types), but the origin is actually the user incorrectly passing bad arguments (i.e. arguments that fails at parsing). So it must not be catch by any except clause inside ours code (maybe only if to beautify the error making it better to understand and debug and re-raise). |
|
Hi @JarrettSJohnson, do you have any other questions? I'm still unsure on how to promote this feature into the community. However I think that it unlock a lot, like better generated documentation and maybe even better command line features like auto-completion. |
|
I'll take most of the fault here, but I probably should have better anticipated how much complexity would be involved for a project like this. I'm definitely in favor of high-level goals here, but I wonder if we should have spent more time in the beginning with a clearer design doc and possible implementation off-repo (ideally in a branch) that we could iterate through before jumping into an implementation that's merged into master. One of the things that I wonder is if a lot of this type checking could have been assisted by libraries like IMO, I think at this point, one of the best ways forward is perhaps to mark this as experimental for this upcoming release and reassess afterward--during which we can test it out on some simple plugins. Alternatively we could move As far as advertisement, I think PyMOLWiki is sufficient; though you may need to be your best advocate for something like this and test it on your plugins so we can see observe its strengths. I'd like to cc the Thomas's @TstewDev and @speleo3 if they have any additional high-level comments or concerns. There's some details in the code that I can point for improvements, but I can follow up on those after once we align on a direction. |
|
Be tranquility. I did a lot of laboratory/testing before myself (e.g. parsing the abstract source tree and PyMOL style docstrings) and opted by this design and like the current overall design even if it's also a bit more complicated then I wanted. Very much like |
|
Take a look on the wiki: https://pymolwiki.org/index.php/New_Command. My specification is a |
|
Click is nice, they have some nice features like @click.command()
@click.argument('input', type=click.File('rb'))
@click.argument('output', type=click.File('wb'))
def inout(input, output):
"""Copy contents of INPUT to OUTPUT."""
while True:
chunk = input.read(1024)
if not chunk:
break
output.write(chunk)@click.command()
@click.argument('filename', type=click.Path(exists=True))
def touch(filename):
"""Print FILENAME if the file exists."""
click.echo(click.format_filename(filename)) |
|
These are components every human-computer interface has to handle case-by-case, while nobody invented something really abstract about this, we'll continuing to re-inventing the interfaces: click, Qt, Jupyter Widgets, Shiny... because of this I think we should focus on PyMOL specific stuff. |
2 participants
No description provided.