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

Adds Is alternate, shorter API #14

Merged
merged 14 commits into from Mar 13, 2020
Merged

Adds Is alternate, shorter API #14

merged 14 commits into from Mar 13, 2020

Conversation

mikeckennedy
Copy link
Contributor

Hi guys. This is in response to issue #12 . Let me know what you think. I tried to stick mostly to the syntax of Check() but in a few places slight rephrasing seemed better. One example of that is going from Check(obj).is_not_zero() -> Is(obj).nonzero as that's the common mathematical term for that check.

@mikeckennedy
Copy link
Contributor Author

One other note, this adds a finish() method. This is totally not needed, but because linters often warn if you do this:

is(n).not_none.float.nonzero

It'll say your variable expression has no effect.

So if people want, they can discourage the linter with:

is(n).not_none.float.nonzero.finish()

finish returns None as well so it stops the fluent API chain.

@deanagan
Copy link
Collaborator

I see our approach here as being possibly cross-breed. So I think we might not need finish. We could possibly set it up like
is(n).not_none.float.nonzero()

This way we get the best of both sides. We somehow need a great documentation for all these though.

@deanagan
Copy link
Collaborator

I see our approach here as being possibly cross-breed. So I think we might not need finish. We could possibly set it up like
is(n).not_none.float.nonzero()

I just realised this still doesn't fix the issue of the linter complaint.

@csparpa
Copy link
Owner

csparpa commented Mar 11, 2020

Cool @mikeckennedy !

Here are my 4 points:

  1. We also need docs... can you pls patch README.md as well to clearly explain users how to use the Is syntax and that it is an alternate option to the Check one?

  2. As regards linter compliants: please specify that as well in the docs

  3. Add yourself into the CONTRIBUTORS.md file!

  4. Want to become a permanent contributor to the project?

Thank you very much!

@mikeckennedy
Copy link
Contributor Author

Thanks, will do. Just found a clever way to avoid the linter issues, see the latest commit just above.

@mikeckennedy
Copy link
Contributor Author

As I make changes to the docs, do you want the shorter is syntax to be the default or secondary as I present them?

@csparpa
Copy link
Owner

csparpa commented Mar 12, 2020

As you like it!

@mikeckennedy
Copy link
Contributor Author

Hi @csparpa I think the PR is ready. Can you let me know if you agree?

@mikeckennedy
Copy link
Contributor Author

mikeckennedy commented Mar 12, 2020

Note: I uncovered some bugs while at it. For example, there are no tests for XML in the Check API and if I try to test it, I get this:

# Test code
def test_is_xml_fail(self):
    obj = "not xml"
    with self.assertRaises(CheckError):
        Is(obj).xml

Results in this failing! with the error:

xml.etree.ElementTree.ParseError: syntax error: line 1, column 0

Rather than succeeding by throwing a CheckError.

But it's not relevant to this PR as we are just extending Check and it's Check that's busted here. Same for YAML and other things like uppercase.

Have a look at test_strings_is.py to see a bunch of TODOs highlighting the broken bits in Check()

@csparpa csparpa merged commit 9c2ecf8 into csparpa:master Mar 13, 2020
@csparpa
Copy link
Owner

csparpa commented Mar 13, 2020

Perfect job! Thanks!!!
I'll take a look to the failing tests, thanks for commenting them out

@mikeckennedy
Copy link
Contributor Author

Thanks @csparpa Happy to give this back to this project. As for being a full contributor, maybe, but not yet. Let me add a little more and if I keep at it, I'm happy to be given more status there.

@mikeckennedy
Copy link
Contributor Author

Also, just to be clear, the failing tests are features I started adding because I saw them in the documentation but not the Check test suite. But once I added the documented but untested features, some of them were broken.

@mikeckennedy
Copy link
Contributor Author

Finally, can you let me know when the new version is on PyPI so I can try the changes in this PR on a few projects?

@csparpa
Copy link
Owner

csparpa commented Mar 13, 2020

No probs, I've fixed the broken tests.
There are many assertions that go untested, we need an incremental effort to extend test coverage - you might help with that.
I've also refactored the Is code to adhere to the same module design as for Check
Coming to PyPI in 3...2...1....

@mikeckennedy
Copy link
Contributor Author

Hi @csparpa

That latest change you made to drop all the actual properties and methods from class Is super broke the IDE and type checker integration.

Here it is on my branch:

Screen Shot 2020-03-13 at 10 32 15 AM

And here it is with this "improvement":

Screen Shot 2020-03-13 at 10 31 54 AM

Please, please, please put it back? It won't work with mypy, it fails with pycharm and vs code, etc etc.

@csparpa
Copy link
Owner

csparpa commented Mar 13, 2020

It's on PyPI

:-( Sorry about it!
I have to say that I've put a lot of work into that refactoring and I wouldn't revert if is a valid alternative can be found. And there is a reason for splitting assertions into separate files: readability and testability - we would otherwise have one HUGE class with hundreds of lines of code, difficult to maintain

@mikeckennedy don't want to bother you too much but.. Can you investigate if IDEs code completion can work properly with the splitted code structure?

I want reverting to be the very last resort

@mikeckennedy
Copy link
Contributor Author

mikeckennedy commented Mar 13, 2020

I think you can make it work super easy but you'll just have to create the Is class differently. Rather than being super dynamic, just make a set of subclasses in the assersions. So for example:

class __IsDictAsserts:
    def dict(check_obj) -> "Is":
        # noinspection PyUnresolvedReferences
        Check(check_obj.object).is_dict()
        return check_obj

    def not_dict(check_obj) -> "Is":
        # noinspection PyUnresolvedReferences
        Check(check_obj.object).is_not_dict()
        return check_obj
   # ...

class __IsStringsAssert:
   # ....

Then create Is as:

class Is(__IsDictAsserts, __IsStringsAssert):
   pass

That will give you type checking, IDE support, code navigation, etc, without a huge class. What do you think?

BTW, you should also do this for Check because it suffers the same shortcomings.

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

Successfully merging this pull request may close these issues.

None yet

3 participants