Feeds #16

danpalmer · 2017-12-27T23:54:27Z

https://app.asana.com/0/465891412411656/496393890703246

coveralls · 2018-01-03T17:00:56Z

Coverage decreased (-1.3%) to 98.439% when pulling a3352a0 on feeds into 9c9786c on master.

There's an open issue on mypy for when there are circular imports AND re-exporting a name. This appears to be what we're seeing here. python/mypy#4049

coveralls · 2018-01-03T18:17:15Z

Coverage decreased (-0.3%) to 99.391% when pulling 8881e33 on feeds into 9c9786c on master.

coveralls · 2018-01-04T14:13:03Z

Coverage decreased (-0.2%) to 99.533% when pulling f366c6c on feeds into 9c9786c on master.

coveralls · 2018-01-04T14:29:07Z

Coverage increased (+0.01%) to 99.733% when pulling fd73449 on feeds into 9c9786c on master.

coveralls · 2018-01-04T16:03:22Z

Coverage increased (+0.01%) to 99.734% when pulling 0446c06 on feeds into 9c9786c on master.

PeterJCLaw

Mostly fine; I think we need to work out how (and test) we're going to handle failures getting data from feeds.

PeterJCLaw · 2018-01-04T15:43:27Z

routemaster/config/tests/test_loading.py

+
+
+def test_multiple_feeds_same_name_invalid():
+    with assert_config_error(f"Feeds must have unique names at state_machines.example.feeds"):


This string doesn't have any format placeholders in -- consider dropping the f prefix.

PeterJCLaw · 2018-01-04T15:46:47Z

routemaster/config/loader.py

+    if len(set(x.name for x in feeds)) < len(feeds):
+        raise ConfigError(
+            f"Feeds must have unique names at {'.'.join(path + ['feeds'])}",
+        )


For even nicer errors, we could use a collections.Counter to detect exactly which feeds had non-unique names. (I'm not sure how many feeds we're expecting to exist in each config file).

I'd expect it will be typically 1, sometimes 1<n<5. I don't think this is necessary for now.

PeterJCLaw · 2018-01-04T15:49:22Z

routemaster/config/model.py

-from routemaster.exit_conditions import Context, ExitConditionProgram
+from routemaster.exit_conditions import ExitConditionProgram
+
+if False:  # typing


What's the rationale behind this being a fake import -- is it circular?

I'm ok with using the if False idiom if there are conflicts on the import or it's a major performance hit, but would rather just import things directly for simpler cases as it's a bit ugly (and also makes the type annotations a bit ugly too).

(future occurrences not commented)

What's the rationale behind this being a fake import -- is it circular?

Yes

This is only to support typing, and in Python 3.7 we can switch to 'if TYPING'.

ok, could we have a comment about these being circular? (possibly as well as the need for it only being for typing).

PeterJCLaw · 2018-01-04T15:54:02Z

routemaster/context.py

+
+    def __init__(
+        self,
+        label: str,


Elsewhere (in the API at least) we call this a label_name (with label meaning a fully qualified name plus state machine), should we use that terminology here too?

Hm, I'm not sure about this. A label should be just the string, I think we should probably change the Label in state_machine to something like LabelRef. Won't do this in this PR though.

PeterJCLaw · 2018-01-04T16:02:55Z

.circleci/config.yml

@@ -45,15 +45,15 @@ jobs:
      # Run tox, caching the .tox directory
      - restore_cache:
          name: Restore .tox cache
-          key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}
+          key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/typechecking/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}


👍 to adding a typechecking requirements.txt; this change (and the similar one below) should probably be cherry-picked to master though.

PeterJCLaw · 2018-01-04T16:12:41Z

routemaster/feeds.py

+from routemaster.utils import get_path
+
+
+def feeds_for_state_machine(state_machine) -> Dict[str, 'Feed']:


Minor: should the argument by type-annotated? (I'm not sure how much typing coverage we're aiming for)

We actually can't here due to python/mypy#4049 – there's detail in the commit message that removed the type.

PeterJCLaw · 2018-01-04T16:14:13Z

routemaster/feeds.py

+        self.data = None
+        self.state_machine = state_machine
+
+    def fetch(self, label: str):


Minor: given that this is lazy and without return value, perhaps it should be named preload or ensure_fetched?

606b673 - prefetch

PeterJCLaw · 2018-01-04T16:14:33Z

routemaster/feeds.py

+            self.state_machine,
+        )
+
+        response = requests.get(url)


What behaviour do we want if this errors? (Should this raise_for_status?)

Yep, this should fail for now. We might want to allow for more in the future, maybe 404 should be allowed for example. 473539d

PeterJCLaw · 2018-01-04T16:30:20Z

routemaster/tests/test_feeds.py

+
+
+@httpretty.activate
+def test_fetch_only_once():


I think this test is a duplicate of test_only_loads_feed_once

Ish. This tests it directly on the feed, the other through a context. Given it's the context's responsibility to pre-warm I think it's worth having this test on both.

PeterJCLaw · 2018-01-04T16:31:13Z

routemaster/tests/test_feeds.py

+
+
+@httpretty.activate
+def test_lookup():


I think this test is a duplicate of test_accesses_variable_in_feed

Not quite, test_accesses_variable_in_feed goes through a layer of dynamic dispatch (in a way) in the context, which is worth testing I think.

coveralls · 2018-01-04T16:52:30Z

Coverage increased (+0.01%) to 99.734% when pulling 473539d on feeds into 9c9786c on master.

PeterJCLaw

Would be great if we could have explicit comments about the typing imports which are circular, fine otherwise.

danpalmer · 2018-01-04T17:52:45Z

Would be great if we could have explicit comments about the typing imports which are circular, fine otherwise.

Do you think this is necessary? I thought the if False pattern was enough to document that.

danpalmer · 2018-01-04T18:02:36Z

Merging now, will address if needed on master.

PeterJCLaw · 2018-01-04T20:19:33Z

I thought the if False pattern was enough to document [that an import is circular and for typing only].

I've seen some projects put all their typing-only imports under this sort of guard, which I'm not sure I see the point of. Given that we only use it for that reason here we maybe don't, but I was then surprised to see that you'd commented them as # typing which is similarly implied.

If we're aiming to generally put typing imports unguarded (which is the style I've used elsewhere and would encourage; from what I've seen it's what we're doing here anyway), then the cases where we're not doing that are interesting for the reason we're not doing that rather than for the fact that they're typing imports. Thus a comment about them being circular is most useful. I'm open to having a comment which says something like "circular typing-only imports" if you feel we need to call out that they're typing imports.

First pass at feeds implementation

ae2ef0b

danpalmer added the Work in Progress label Dec 27, 2017

danpalmer added this to the Internal Production Use milestone Dec 27, 2017

danpalmer added 6 commits December 27, 2017 23:57

We need requests for the feeds

1bea78b

Merge branch 'master' into feeds

e02f519

Merge branch 'master' into feeds

76b7739

Linter

ba6a996

Move Context, it's now not just axout execution

36bbabf

Update tests for new constructor args

a3352a0

danpalmer added 10 commits January 3, 2018 17:48

This interferes with other stuff, remove for now

7079d5b

We need the label to template the URL

f83cf76

Fix circular imports for types

8254e54

Update tests for new Context constructor

ee634af

Test the feed access

58d2723

Ensure we only get the data once

2e5398d

Linting

64ecec6

Add some types

87b710c

Move mypy dependency into a file

6526f06

Drop types for this for now

8881e33

There's an open issue on mypy for when there are circular imports AND re-exporting a name. This appears to be what we're seeing here. python/mypy#4049

danpalmer added 4 commits January 3, 2018 18:24

Make these tests more realistic

29fb2af

Add feed config

8b5b93a

Test that feeds must have unique names

12aeee9

Implement and test setup for feeds

f366c6c

danpalmer added 3 commits January 4, 2018 14:27

Call correctly

b9f1924

Improve behaviour of this util

62eeacf

More context tests

fd73449

danpalmer requested a review from PeterJCLaw January 4, 2018 14:33

Support state machine name in feed URL

0446c06

danpalmer added For Review High Priority and removed Work in Progress labels Jan 4, 2018

PeterJCLaw requested changes Jan 4, 2018

View reviewed changes

danpalmer added 4 commits January 4, 2018 16:39

Remove unnecessary f-string

ae33115

Types

e485628

Rename for clarity

606b673

Should fail loudly if the feed errors

473539d

PeterJCLaw approved these changes Jan 4, 2018

View reviewed changes

danpalmer merged commit 8059262 into master Jan 4, 2018

danpalmer deleted the feeds branch January 4, 2018 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feeds #16

Feeds #16

danpalmer commented Dec 27, 2017 •

edited

coveralls commented Jan 3, 2018

coveralls commented Jan 3, 2018

coveralls commented Jan 4, 2018

coveralls commented Jan 4, 2018

coveralls commented Jan 4, 2018

PeterJCLaw left a comment

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

PeterJCLaw Jan 4, 2018

danpalmer Jan 4, 2018

coveralls commented Jan 4, 2018

PeterJCLaw left a comment

danpalmer commented Jan 4, 2018

danpalmer commented Jan 4, 2018

PeterJCLaw commented Jan 4, 2018 •

edited



		def test_multiple_feeds_same_name_invalid():
		with assert_config_error(f"Feeds must have unique names at state_machines.example.feeds"):

		from routemaster.utils import get_path


		def feeds_for_state_machine(state_machine) -> Dict[str, 'Feed']:

Feeds #16

Feeds #16

Conversation

danpalmer commented Dec 27, 2017 • edited

coveralls commented Jan 3, 2018

coveralls commented Jan 3, 2018

coveralls commented Jan 4, 2018

coveralls commented Jan 4, 2018

coveralls commented Jan 4, 2018

PeterJCLaw 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

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

coveralls commented Jan 4, 2018

PeterJCLaw left a comment

Choose a reason for hiding this comment

danpalmer commented Jan 4, 2018

danpalmer commented Jan 4, 2018

PeterJCLaw commented Jan 4, 2018 • edited

danpalmer commented Dec 27, 2017 •

edited

PeterJCLaw commented Jan 4, 2018 •

edited