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

Code Review #1

Open
2 tasks
Ayplow opened this issue Aug 21, 2019 · 4 comments
Open
2 tasks

Code Review #1

Ayplow opened this issue Aug 21, 2019 · 4 comments

Comments

@Ayplow
Copy link
Contributor

Ayplow commented Aug 21, 2019

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

raise UnclosedQuote("Missing closing quote.")

  • Variable doesn't exist

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

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)

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.

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.

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

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

Zomatree added a commit that referenced this issue Aug 22, 2019
Zomatree added a commit that referenced this issue Aug 22, 2019
@Zomatree
Copy link
Member

b67807b

@Ayplow
Copy link
Contributor Author

Ayplow commented Oct 1, 2019

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
Copy link
Member

Zomatree commented Oct 1, 2019

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

@Zomatree
Copy link
Member

Zomatree commented Oct 2, 2019

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

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

No branches or pull requests

2 participants