Implement parsing logic for converting text to Expression #1079

kitchoi · 2020-05-12T17:01:20Z

This PR works towards finishing item 4 in #977

Text parser for parsing traits domain specific language to an instance of Expression

Add conversion logic from a syntax tree to an instance of Expression
Add parse function to convert text to Expression. This function is exposed in the public API
Add trait as a method on Expression for extending an expression for observing a trait with a given name.
Add trait top-level function for creating an expression for a given trait name (this just wraps the method on an empty expression).

Note that parsing "+metadata" and "items" currently raise NotImplementedError as the observers required for these two are still being reviewed. This PR therefore adds some placeholders for them.

Checklist

Tests
Update API reference (docs/source/traits_api_reference)
~~Update User manual (docs/source/traits_user_manual)~~
~~Update type annotation hints in traits-stubs~~

traits/observers/parsing.py

traits/observers/tests/test_parsing.py

mdickinson · 2020-05-13T11:48:42Z

Assigning to myself for review.

mdickinson · 2020-05-14T09:59:35Z

traits/observers/expression.py

+
+        e.g. ``trait("child").trait("age")`` matches ``child.age``
+        on an object, and is equivalent to
+        ``trait("child").then(trait("age"))``


Could we stick with just one way to do it here? I think trait("child").then(trait("age")) should be fine for now; we could add the trait("child").trait("age") support later if we think it's needed.

In some sense, this is a violation of the OCP: then is a general piece of machinery that lets us chain expressions, while if we follow this pattern we'll want an extra method for each new type of observer - we end up repeating ourselves, and each new observer type expects a new method in this class.

Ah, I see, there isn't a repetition here, because of the _as_top_level magic that converts the method into a matching function. I'd recommend removing the magic and the method, and having a direct top-level function for creating the expression. That'll make for simpler, more direct code that's easier to maintain going forward.

This is very much a user interface (just not graphical or command-line-like), a bit like a main application. It seems laborious to do trait("child").then(trait("attr1")).then(trait("attr2")).then(trait("attr3")) when we could do trait("child").trait("attr").trait("attr2")

Likewise, this is more user-friendly:
trait("child").metadata("name")
than
trait("child").then(metadata("name"))
The latter also requires importing metadata in addition to trait.

@corranwebster Since you have reviewed the user experience before, thought I should check again if we are changing this. Is it okay if we ditch trait("name1").trait("name2").trait("name3") but require the user to go with trait("name1").then(trait("name2")).then(trait("name3")) instead?

(This will apply to all the other features, e.g. metadata and filter_)

I am fine with this sort of change to the API.

(This means a lot of the tests I have written for future PRs will need to change. Sigh.)

I'm fine with keeping the .trait method, FWIW, just so long as we untangle so that it's clear that it's the .trait method that depends on the function, and not the other way around.

Thanks. I will keep that then.

mdickinson · 2020-05-14T10:10:05Z

traits/observers/parsing.py

+    """ Handle trees when the notify is not immediately specified
+    as a suffix. The last notify flag will be used.
+
+    e.g. In "a.[b,c]:.d", the element "b" should receive a notify flag


Is the :. here intentional, or a typo?

Is a typo. Thanks!

mdickinson · 2020-05-14T10:24:14Z

traits/observers/parsing.py

+
+import traits.observers.expression as _expr_module
+from traits.observers._generated_parser import (
+    Lark_StandAlone as _Lark_StandAlone


Oh my, that's horrible. Camel_CaseWithUnderscores? Not much we can do about that, I guess.

Yep, the name in the generated parser is out of our control. I suppose we could change the alias name...

I'm curious: why are we aliasing this in the first place?

Because this parsing is a public module, and I got overly excited about hiding variables from TAB auto-completion... except I failed to do this 100% because the standard library imports aren't given private names.

Okay. We don't have any policy in place about doing this in general; I think that given that we rarely ever do a from ... import *, it's probably not needed.

mdickinson · 2020-05-14T10:26:11Z

traits/observers/parsing.py

+_LARK_PARSER = _Lark_StandAlone()
+
+#: Token annotation for a name (a trait name, or a metadata name, etc.)
+_NAME_TOKEN = "NAME"


Is this something we should be importing from somewhere? Where/how does Lark store the string constants it uses for token types?

This "NAME" is data from the grammar, and so it is going to be somewhere in the generated code DATA or MEMO variable. However those two structures are not really for human consumption.

On the other hand, this _NAME_TOKEN is here only for sanity check. I can remove it too and just not check it.

It's a bit odd if Lark exposes token.type as part of the public API but doesn't expose the constants for token.type to be compared to. How are consumers supposed to use token.type?

Sorry for not being clear...
I meant the "NAME" is defined by us:

traits/traits/observers/_dsl_grammar.lark

Line 51 in db76bc8

NAME: /[a-zA-Z_]\w*/

If I change the grammar file such that "NAME: /[a-zA-Z_]\w*/" becomes "NAME2: /[a-zA-Z_]\w*/" (and everywhere else that uses it), then regenerate the standalone parser. This _NAME_TOKEN = "NAME" will need to change to _NAME_TOKEN = "NAME2".

If it is okay, I am going to remove this sanity check. It is on the border of being not really necessary given we already have tests for the parsed outcomes.

The sanity check for "NAME" is not covered in tests. Likewise, this line is not covered either.

traits/traits/observers/parsing.py

Line 87 in ab16458

raise RuntimeError("Default notify flag unexpectedly changed.")

mdickinson

Mostly LGTM. I think this is baking in some of the Expression logic that we're going to want to change before the release, but that may be clearer when everything's together.

Please could we move the logic for the trait function into the function itself, and modify the trait method to use that function rather than the other way around? I think that'll let us get rid of the decorator magic, and leave us better prepared for the later changes.

This reverts commit fb80fa8.

…rrectly.

kitchoi · 2020-05-14T12:43:12Z

Thank you for the review. I hope I have addressed all the comments.

mdickinson

LGTM

kitchoi · 2020-05-14T14:37:43Z

Thank you. Merging...

kitchoi added 6 commits May 12, 2020 17:41

Implement parsing logic to create an instance of Expresion from text

80a19b5

Remove a comment

fe21b12

Expand docstring to mention top-level functions

b7421f9

Fix spelling

62d502d

Fix missing parameter in docstring

203d884

Expose parse to the API doc

4afce54

kitchoi changed the title ~~Implement parsing logic for creating an instance of Expression~~ Implement parsing logic for converting text to Expression May 12, 2020

kitchoi commented May 12, 2020

View reviewed changes

traits/observers/parsing.py Outdated Show resolved Hide resolved

Fix docstring due to renamed module

89ac12b

kitchoi commented May 12, 2020

View reviewed changes

traits/observers/tests/test_parsing.py Outdated Show resolved Hide resolved

Remove repeated tests

08125db

kitchoi mentioned this pull request May 12, 2020

Implement new framework to observe and notify for trait changes #977

Closed

19 tasks

Add tests for Expression.trait

ab16458

mdickinson self-assigned this May 13, 2020

mdickinson self-requested a review May 13, 2020 12:32

mdickinson reviewed May 14, 2020

View reviewed changes

mdickinson requested changes May 14, 2020

View reviewed changes

kitchoi added 9 commits May 14, 2020 12:09

Remove trait method, keep only the top-level function.

3884c4e

Update test to use then instead of trait method

fb80fa8

Revert "Update test to use then instead of trait method"

c4a2932

This reverts commit fb80fa8.

Add method to call top-level, add the tests to ensure it is called co…

98864ee

…rrectly.

Add test to help keep the two functions in sync

1c43bf4

Fix a typo

0f5649b

Improve comment

4861032

Remove sanity check on 'NAME'

d65771b

Flake8

feca25d

kitchoi requested a review from mdickinson May 14, 2020 12:43

mdickinson approved these changes May 14, 2020

View reviewed changes

kitchoi merged commit fa56823 into master May 14, 2020

kitchoi deleted the 977-parsing branch May 14, 2020 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement parsing logic for converting text to Expression #1079

Implement parsing logic for converting text to Expression #1079

kitchoi commented May 12, 2020

mdickinson commented May 13, 2020

mdickinson May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020 •

edited

Loading

kitchoi May 14, 2020

kitchoi May 14, 2020

corranwebster May 14, 2020

kitchoi May 14, 2020

kitchoi May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020

mdickinson May 14, 2020

mdickinson May 14, 2020 •

edited

Loading

kitchoi May 14, 2020

mdickinson May 14, 2020

kitchoi May 14, 2020

kitchoi May 14, 2020

kitchoi May 14, 2020 •

edited

Loading

mdickinson left a comment •

edited

Loading

kitchoi commented May 14, 2020

mdickinson left a comment

kitchoi commented May 14, 2020

Implement parsing logic for converting text to Expression #1079

Implement parsing logic for converting text to Expression #1079

Conversation

kitchoi commented May 12, 2020

mdickinson commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitchoi May 14, 2020 • edited Loading

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

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

mdickinson May 14, 2020 • edited Loading

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

kitchoi May 14, 2020 • edited Loading

Choose a reason for hiding this comment

mdickinson left a comment • edited Loading

Choose a reason for hiding this comment

kitchoi commented May 14, 2020

mdickinson left a comment

Choose a reason for hiding this comment

kitchoi commented May 14, 2020

kitchoi May 14, 2020 •

edited

Loading

mdickinson May 14, 2020 •

edited

Loading

kitchoi May 14, 2020 •

edited

Loading

mdickinson left a comment •

edited

Loading