csparpa / fluentcheck

Fluent assertions for Python
MIT License
84 stars 8 forks source link

Adds Is alternate, shorter API #14

Closed mikeckennedy closed 4 years ago

mikeckennedy commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mikeckennedy commented 4 years ago

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 commented 4 years ago

As you like it!

mikeckennedy commented 4 years ago

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

mikeckennedy commented 4 years ago

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 commented 4 years ago

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

mikeckennedy commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.