-
Notifications
You must be signed in to change notification settings - Fork 328
Improvements on the @cmd.declare_command API
#448
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
095e961
d0f8406
40d5053
d9dec36
9fc0304
3140e28
3aa351b
a78deb4
3612302
b2493c7
ab4c87e
22ba8f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,16 +21,19 @@ | |||||||||||||||||||||
| if True: | ||||||||||||||||||||||
| import _thread as thread | ||||||||||||||||||||||
| import urllib.request as urllib2 | ||||||||||||||||||||||
| from io import FileIO as file | ||||||||||||||||||||||
| from io import FileIO as file, BytesIO | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import builtins | ||||||||||||||||||||||
| import inspect | ||||||||||||||||||||||
| import glob | ||||||||||||||||||||||
| import shlex | ||||||||||||||||||||||
| import tokenize | ||||||||||||||||||||||
| from enum import Enum | ||||||||||||||||||||||
| from functools import wraps | ||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||
| from textwrap import dedent | ||||||||||||||||||||||
| from typing import List | ||||||||||||||||||||||
| from typing import Tuple, Iterable, get_args, Optional, Union, Any, NewType, List, get_origin | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import re | ||||||||||||||||||||||
| import os | ||||||||||||||||||||||
|
|
@@ -600,45 +603,128 @@ def get_state_list(states_str): | |||||||||||||||||||||
| states_list = sorted(set(map(int, output))) | ||||||||||||||||||||||
| return _cmd.delete_states(_self._COb, name, states_list) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class Selection(str): | ||||||||||||||||||||||
| pass | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _parse_bool(value: str): | ||||||||||||||||||||||
| if isinstance(value, str): | ||||||||||||||||||||||
| if value.lower() in ["yes", "1", "true", "on", "y"]: | ||||||||||||||||||||||
| return True | ||||||||||||||||||||||
| elif value.lower() in ["no", "0", "false", "off", "n"]: | ||||||||||||||||||||||
| return False | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| raise Exception("Invalid boolean value: %s" % value) | ||||||||||||||||||||||
| elif isinstance(value, bool): | ||||||||||||||||||||||
| def _into_types(type, value): | ||||||||||||||||||||||
| if repr(type) == 'typing.Any': | ||||||||||||||||||||||
| return value | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| raise Exception(f"Unsuported boolean flag {value}") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _parse_list_str(value): | ||||||||||||||||||||||
| return shlex.split(value) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| elif type is bool: | ||||||||||||||||||||||
| if isinstance(value, bool): | ||||||||||||||||||||||
| return value | ||||||||||||||||||||||
| elif isinstance(value, str): | ||||||||||||||||||||||
| if value.lower() in ["yes", "1", "true", "on", "y"]: | ||||||||||||||||||||||
| return True | ||||||||||||||||||||||
| elif value.lower() in ["no", "0", "false", "off", "n"]: | ||||||||||||||||||||||
| return False | ||||||||||||||||||||||
| elif isinstance(value, int): | ||||||||||||||||||||||
| if value in [0, 1]: | ||||||||||||||||||||||
pslacerda marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||
| return bool(value) | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| raise pymol.CmdException("Invalid boolean value: %s" % value) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _parse_list_int(value): | ||||||||||||||||||||||
| return list(map(int, shlex.split(value))) | ||||||||||||||||||||||
| elif isinstance(type, Enum): | ||||||||||||||||||||||
|
||||||||||||||||||||||
| if value in type: | ||||||||||||||||||||||
| return type(value) | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| raise pymol.CmdException(f"Invalid value for enum {type.__name__}: {value}") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| elif isinstance(type, builtins.type): | ||||||||||||||||||||||
|
||||||||||||||||||||||
| elif isinstance(type, builtins.type): | |
| elif inspect.isclass(type): |
pslacerda marked this conversation as resolved.
Show resolved
Hide resolved
pslacerda marked this conversation as resolved.
Show resolved
Hide resolved
pslacerda marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function probably needs its own unit test as well. The API to me is not inherently clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is tested integrated with declare_command on test_declare_command_arg_docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but something like this probably deserves its own test too if possible. I'll make it optional, but I will likely add it later.
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause an infinite loop if the opening parenthesis is found, as the condition never changes. The logic should find the opening parenthesis and then break or continue processing.
| while tokens[i].string == "(": | |
| if tokens[i].string == "(": |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop may cause an IndexError when accessing tokens[i+1] if i reaches the end of the tokens list. Add bounds checking before accessing tokens[i+1].
| if tokens[i].type == tokenize.NAME and tokens[i+1].string == ":": | |
| name = tokens[i].string | |
| name_line = tokens[i].line | |
| i += 1 | |
| while not (tokens[i].type == tokenize.NAME and tokens[i+1].string == ":"): | |
| if tokens[i].type == tokenize.NAME and (i+1 < len(tokens)) and tokens[i+1].string == ":": | |
| name = tokens[i].string | |
| name_line = tokens[i].line | |
| i += 1 | |
| while not (tokens[i].type == tokenize.NAME and (i+1 < len(tokens)) and tokens[i+1].string == ":"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I follow this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither, I just placed it here because was needed. I'd need to debug to remember exactly, but seems to me that it's related to rewind the cursor i when no doc is found.
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent spacing around commas. Should be [inner, 0, 0, ',', parsing.STRICT] to match Python style conventions.
| _self.keyword[name] = [inner, 0,0,',',parsing.STRICT] | |
| _self.keyword[name] = [inner, 0, 0, ',', parsing.STRICT] |
Uh oh!
There was an error while loading. Please reload this page.