Add action flag to eval#677
Conversation
scragly
left a comment
scragly
left a comment
There was a problem hiding this comment.
The implementation uses an unnecessary imagining for commands processing with this Literal converter that's handling something entirely capable of being handled by commands groups, which are already well used elsewhere in the project.
This PR has also not been linted before being created. This is not the first time you've created a PR that has lacked the basic care that's expected when contributing.
Due to the above considerations, I'm closing the PR until you can correctly setup your environment, linter, pre-commit hook and take into consideration the existing framework and project style, all of which are things mentioned in our contributing guidelines.
| await ctx.invoke(self.bot.get_command("help"), "eval") | ||
| return | ||
|
|
||
| if action == "-save": |
There was a problem hiding this comment.
This entire thing appears to be a poor-mans subcommand handler. There's no reason to parse distinct flags contrary to the entire commands framework design when we have the ability to add subcommands through usage of a command group object.
| # Dict[int[user_id], | ||
| # Tuple[str[saved_code], | ||
| # datetime[last_used]]] |
There was a problem hiding this comment.
Don't randomly leave commented out code in your PRs.
There was a problem hiding this comment.
This isn't commented out code, this is a comment explaining the type used so other people working on this know what it is.
| async def eval_command(self, ctx: Context, action: Optional[Literal["-save", "-load"]] = None, | ||
| *, code: str = None) -> None: |
There was a problem hiding this comment.
This does not conform to typical project style. Multiline arguments are dropped after the brackets:
async def eval_command(
self, ctx: Context, action: Optional[Literal["-save", "-load"]] = None, *, code: str = None
) -> None:There was a problem hiding this comment.
Doesn't this fprmat violate PEP8?
There was a problem hiding this comment.
No.
The closing brace/bracket/parenthesis on multi-line constructs may either line up under the first non-whitespace character of the last line of list
or it may be lined up under the first character of the line that starts the multi-line construct, as in:
my_list = [
1, 2, 3,
4, 5, 6,
]
result = some_function_that_takes_arguments(
'a', 'b', 'c',
'd', 'e', 'f',
)Even if it wasn't specifically allowed, it wouldn't have violated any of it's forbiddens, and in the end PEP8 is a guide and the existing project style is the determining factor.
| class Literal: | ||
| """ | ||
| Matches a literal, useful for e.g. flags. | ||
| Can be used with Optional. | ||
|
|
||
| Implementation by Danny#0007 | ||
| """ | ||
|
|
||
| def __class_getitem__(cls, item): | ||
| if not isinstance(item, tuple): | ||
| item = (item,) | ||
|
|
||
| class LiteralProxy(Converter): | ||
| @classmethod | ||
| async def convert(cls, ctx, argument): | ||
| if argument in item: | ||
| return argument | ||
| raise BadArgument(f"Expected literal: one of {list(map(repr, self.literals))}") | ||
| return LiteralProxy |
There was a problem hiding this comment.
Unnecessary to add at this point in time due to suggesting to change commands processing to the existing command groups format.
| class Literal: | ||
| """ | ||
| Matches a literal, useful for e.g. flags. | ||
| Can be used with Optional. |
There was a problem hiding this comment.
Linter: D205 1 blank line required between summary line and description
| self.contexts = {} | ||
| self._run = True | ||
|
|
||
| async def clean_task(): |
There was a problem hiding this comment.
Linter: TYP201 Missing return type annotation for public function
Also where's the docstring here.
MarkKoz
added
a: utility
3 participants
Literal(requires 3.7+, implementation by Danny#0007)