Code Review

[Ayplow]

For c4ad472

• No test suite
• No linting

Solving the above will force most of the other problems to be solved, but here are some others that popped up

Actioneer/actioneer/performer.py

Line 50 in 874ddc1

raise UnclosedQuote("Missing closing quote.")

• Variable doesn't exist

Actioneer/actioneer/performer.py

Line 70 in 874ddc1

del inp[i]

Removing items from the middle of a list is a very slow method of skipping through elements.

Actioneer/actioneer/performer.py

Line 66 in 874ddc1

if not arg.startswith("--"): continue

The difference between flags and options in your parser seems to be the prefix - - and -- (hardcoded at the moment) This is not convention - it is more generally expected that a single dash is used for the shorthand of either an option (-m=utf) or a flag (-f)

Actioneer/actioneer/action.py

Lines 19 to 22 in 874ddc1

	self.options = options
	self.options_aliases = options_aliases
	self.flags = flags
	self.flags_aliases = flags_aliases

This data structure is larger than necessary, forcing consumers to use both dictionaries. It would be better to choose one - either iteration through values that declared their aliases or a map of all flags, with some keys holding identical values.

Actioneer/actioneer/action.py

Line 33 in 874ddc1

ctx = {type(a): a for a in ctx}

This form of lookup precludes a major use of these context annotations - subclassing. As we drop all information about the mro to do a property access, only the direct constructor of the value is relevant.

Actioneer/actioneer/performer.py

Line 32 in 874ddc1

raise Exception("BRUH") # TODO: do custom error class

Yeah, do. This is a decent chunk of the library just missing

[Zomatree]

b67807b

[Ayplow]

Saw you wondering what else needed doing in the library in discord, thought a response fit here

The original recommendations stand. Testing is important - write unit tests for behaviour you care about, and integration tests to make things don't fall apart when you try to actually use them. A simple command line Performer would be useful for writing tests with, and an invaluable example implementation of the library.

As for why proper testing is important - at a glance, https://github.com/clamor-py/Actioneer/blob/master/actioneer/performer.py#L72 is just broken - functions arent awaitable, their return values are. Writing a full test suite would solve this problem, but there are ways to find where you're lacking;

When writing tests, we can check their 'coverage' of our codebase, finding out how much of our code is tested, and what needs to be looked at. By the looks of it, Coverage.py is the popular python library for doing this.

As a bare minimum (ideally your test cases touch on uncommon scenarios too), make sure the result of https://github.com/audreyr/how-to/blob/master/python/use_coverage_with_unittest.rst is >80%. It should probably be higher with something like actioneer, but thats up to you.

[Zomatree]

I have pushes I haven't done, and I'm making a bot for testing it

[Zomatree]

4130d7a, ive done the fix you mentioned, and i do have some tests for it just not pushed, feel free to make some and submit them, the more there are the better we can find those bugs