PR: Add clip and overflow props #57

goanpeca · 2020-01-20T06:12:28Z

No description provided.

freakboy3742

Looks good on the whole; but the parser for rect needs a bit more work.

freakboy3742 · 2020-01-22T09:23:55Z

colosseum/validators.py

+        value = value.strip()
+
+        if value.startswith('rect'):
+            value = value.replace('rect(', '').replace(')', '')


There's a few holes in the parsing scheme. If you don't have a closing brace, it will succeed; if you don't have an opening brace, it will fail (but only because the first value won't be parsed cleanly. I understand the desire to keep this parsing scheme simple (and not invoke regexes), but this needs to be a little more robust.

Move to use a regex 🤷‍♂...I guess it is simple and robust enough for this case?

freakboy3742 · 2020-01-22T09:25:42Z

tests/test_validators.py

+            is_shape('(1px, 3px, 2px, 4px)')
+
+        with self.assertRaises(ValidationError):
+            is_shape('rect(a, b, c, d)')


Some examples of "badly composed attempts at writing rect()" would be helpful here - e.g., mixing spaces and commas, missing open/close brace,...

Updated tests

goanpeca · 2020-01-23T02:36:21Z

Looks good on the whole; but the parser for rect needs a bit more work.

@freakboy3742 I updated the code to use a regex. It is way more robust and yet simple enough.

Thanks for the review! I left a couple of questions

goanpeca · 2020-01-23T02:43:29Z

colosseum/parser.py

+
+def rect(value):
+    """Parse a given rect shape."""
+    from .shapes import Rect


This is here because the shape uses units, so to prevent circular imports.

I could not use shapes.Rect here but use it on validators and have the Rect import there, or I could move the Rect shape to this file.

Thoughts @freakboy3742 ?

Another (any my preferred) approach would be to have the constructor for Rect take fully parsed unit values, rather than deferring part of the parsing to the Rect constructor.

freakboy3742

This is starting to look pretty good. I probably would have stayed with a simple string parsing approach rather than the regex - but the regex isn't too inscrutable, so I can live with that.

A couple of comments inline - including a recurring theme about splitting up test cases. This sort of test is what pytest parametrization is really good at - which is one of the reasons I've become a fan.

freakboy3742 · 2020-01-23T04:58:54Z

colosseum/parser.py

+
+def rect(value):
+    """Parse a given rect shape."""
+    from .shapes import Rect


Another (any my preferred) approach would be to have the constructor for Rect take fully parsed unit values, rather than deferring part of the parsing to the Rect constructor.

freakboy3742 · 2020-01-23T05:04:19Z

tests/test_parser.py

+class ParseRectTests(TestCase):
+
+    def test_rect_valid(self):
+        # Comma separated


Insert comment about separating tests here. :-)

Split tests

goanpeca · 2020-01-23T05:14:47Z

This is starting to look pretty good. I probably would have stayed with a simple string parsing approach rather than the regex - but the regex isn't too inscrutable, so I can live with that.

I can go back to simple string parsing if you really really prefer

goanpeca

Another (any my preferred) approach would be to have the constructor for Rect take fully parsed unit values, rather than deferring part of the parsing to the Rect constructor.

Fixed

This is starting to look pretty good. I probably would have stayed with a simple string parsing approach rather than the regex - but the regex isn't too inscrutable, so I can live with that.

Went back to simple string parsing

A couple of comments inline - including a recurring theme about splitting up test cases. This sort of test is what pytest parametrization is really good at - which is one of the reasons I've become a fan.

I prefer to move to pytest on a separate PR (or the fonts one...) if that is what you were implying ;-)

freakboy3742 · 2020-01-26T03:58:15Z

The reference to tests wasn't specifically about moving to pytest. Moving to PyTest makes test parametrization an option, which makes some of these tests a lot less verbose, but it's possible to split tests so they're more granular without necessarily moving to pytest.

I was more noting that the "this should be N little tests, not one big test" is a comment I've made on a couple of PRs.

freakboy3742

Ok - we're definitely getting close now. We're down to some final small nitpicks.

freakboy3742 · 2020-01-26T03:59:59Z

colosseum/parser.py

+
+def rect(value):
+    """Parse a given rect shape."""
+    value = ' '.join([val.strip() for val in value.split()])


This could be done slightly more efficiently without constructing the inner list :

value = ' '.join(val.strip() for val in value.split())

will behave exactly the same, but it will process the content on the fly, rather than constructing a full list in memory.

freakboy3742 · 2020-01-26T04:04:53Z

colosseum/shapes.py

+        self._bottom = bottom
+
+    def __eq__(self, other):
+        return other.__class__ == self.__class__ and other.to_tuple() == self.to_tuple()


Is this the only reason to have to_tuple? The extra function call isn't free, and the previous check has confirmed both objects are Rect... is there any reason we don't just confirm equality of the four attributes?

Hmmm nope, the only reason was this, yes!

Fixed

freakboy3742 · 2020-01-26T04:07:15Z

colosseum/validators.py

+def is_shape(value):
+    """Check if given value is a shape and return it."""
+    try:
+        value = parser.rect(value)


There's a slight inconsistency in language here - is a rect one of multiple shape options? I know clip-path allows for other options, but in that context rect isn't one of the options. Is there any particular reason to use the word "shape" here? (and also in the exception raised by parser.rect())

goanpeca · 2020-01-26T22:42:15Z

Ok - we're definitely getting close now. We're down to some final small nitpicks.

Fixed!

Thanks for the review @freakboy3742 !

freakboy3742

Looks good! I'd still like to see more granularity in the tests; but I suspect we're at the point where a single PR to introduct pytest and parametrization would be more productive.

goanpeca · 2020-01-27T01:13:47Z

Looks good! I'd still like to see more granularity in the tests; but I suspect we're at the point where a single PR to introduct pytest and parametrization would be more productive.

Agreed, that is on my todo. Opened an #61 to keep track of it.

goanpeca requested a review from freakboy3742 January 20, 2020 06:16

goanpeca force-pushed the enh/add-clip-overflow branch from 0267476 to 7281a1f Compare January 20, 2020 15:00

freakboy3742 requested changes Jan 22, 2020

View reviewed changes

goanpeca added 2 commits January 22, 2020 21:30

Add clip and overflow props

3bbbe50

Use regex for shape validation, add more tests

91f253c

goanpeca force-pushed the enh/add-clip-overflow branch from 0fe1043 to f4f732b Compare January 23, 2020 02:32

Fix code style

acf1e82

goanpeca force-pushed the enh/add-clip-overflow branch from f4f732b to acf1e82 Compare January 23, 2020 02:34

goanpeca requested a review from freakboy3742 January 23, 2020 02:36

goanpeca commented Jan 23, 2020

View reviewed changes

Simplify regex and add more texts

3616823

goanpeca force-pushed the enh/add-clip-overflow branch from 7d54f69 to 3616823 Compare January 23, 2020 03:00

freakboy3742 requested changes Jan 23, 2020

View reviewed changes

goanpeca commented Jan 23, 2020

View reviewed changes

Replace regex by string parsing and split tests

89009e5

goanpeca force-pushed the enh/add-clip-overflow branch from 98b485c to 89009e5 Compare January 23, 2020 06:02

goanpeca requested a review from freakboy3742 January 23, 2020 06:02

goanpeca added 2 commits January 23, 2020 01:19

Simplify parser

8d90568

Extra tests

ad19d5d

freakboy3742 requested changes Jan 26, 2020

View reviewed changes

Rename validator, update tests and srefactor/simplify code

32e98dd

goanpeca requested a review from freakboy3742 January 26, 2020 22:41

freakboy3742 approved these changes Jan 27, 2020

View reviewed changes

freakboy3742 merged commit 7dc27fd into beeware:master Jan 27, 2020

goanpeca deleted the enh/add-clip-overflow branch January 27, 2020 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add clip and overflow props #57

PR: Add clip and overflow props #57

goanpeca commented Jan 20, 2020

freakboy3742 left a comment

freakboy3742 Jan 22, 2020

goanpeca Jan 23, 2020 •

edited

Loading

freakboy3742 Jan 22, 2020

goanpeca Jan 23, 2020 •

edited

Loading

goanpeca commented Jan 23, 2020 •

edited

Loading

goanpeca Jan 23, 2020

freakboy3742 Jan 23, 2020

goanpeca Jan 23, 2020 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Jan 23, 2020

freakboy3742 Jan 23, 2020

goanpeca Jan 23, 2020 •

edited

Loading

goanpeca commented Jan 23, 2020

goanpeca left a comment

freakboy3742 commented Jan 26, 2020

freakboy3742 left a comment

freakboy3742 Jan 26, 2020

goanpeca Jan 26, 2020

goanpeca Jan 26, 2020

freakboy3742 Jan 26, 2020

goanpeca Jan 26, 2020

freakboy3742 Jan 26, 2020

goanpeca Jan 26, 2020

goanpeca commented Jan 26, 2020

freakboy3742 left a comment

goanpeca commented Jan 27, 2020

PR: Add clip and overflow props #57

PR: Add clip and overflow props #57

Conversation

goanpeca commented Jan 20, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

goanpeca commented Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

goanpeca commented Jan 23, 2020

goanpeca left a comment

Choose a reason for hiding this comment

freakboy3742 commented Jan 26, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jan 26, 2020

freakboy3742 left a comment

Choose a reason for hiding this comment

goanpeca commented Jan 27, 2020

goanpeca Jan 23, 2020 •

edited

Loading

goanpeca Jan 23, 2020 •

edited

Loading

goanpeca commented Jan 23, 2020 •

edited

Loading

goanpeca Jan 23, 2020 •

edited

Loading

goanpeca Jan 23, 2020 •

edited

Loading