Fix path_semantics and be robust against empty 'filled'. #59

gwbischof · 2019-03-18T21:58:53Z

No description provided.

…filled key, made a fix for this

gwbischof · 2019-03-19T15:12:59Z

There are so many commits because the tests fail on my windows machine, so I'm using travis until I can fix the windows errors.

event_model/__init__.py

danielballan

Thanks for the fix! Some small suggestions.

event_model/__init__.py

danielballan · 2019-03-25T14:04:14Z

event_model/__init__.py

@@ -499,6 +499,8 @@ def compose_resource(*, start, spec, root, resource_path, resource_kwargs,
                     path_semantics=os.name, uid=None, validate=True):
    if uid is None:
        uid = str(uuid.uuid4())
+    if path_semantics == 'nt':


I worry that this might confuse users who look at the function signature (i.e. compose_resource? in IPython) and find a default valid that is not a valid value. Have you considered doing this translation from os.name to path_semantics above the function definition rather than inside it?

default_path_semantics = {...}[os.name] def compose_resource(..., path_semantics=default_path_semantics, ...)

mrakitin

Looks good to me after @danielballan's suggestions are implemented.
Left one important suggestion regarding paths.

mrakitin · 2019-03-25T15:51:58Z

event_model/tests/test_em.py

@@ -131,8 +146,14 @@ def test_bulk_events_to_event_page():
    desc_bundle_baseline = run_bundle.compose_descriptor(
        data_keys={'motor': {'shape': [], 'dtype': 'number', 'source': '...'}},
        name='baseline')
+
+    if os.name == 'nt':
+        path_root = 'C:\tmp'


To avoid it to be evaluated as "tab" (\t)

Suggested change

path_root = 'C:\tmp'

path_root = 'C:\\tmp'

Quick IPython demo:

In [1]: path_root = 'C:\tmp' In [2]: path_root Out[2]: 'C:\tmp' In [3]: print(path_root) C: mp In [4]: path_root = 'C:\\tmp' In [5]: path_root Out[5]: 'C:\\tmp' In [6]: print(path_root) C:\tmp

prjemian · 2019-03-25T16:23:17Z

Isn't there an OS specific python term for the temporary directory? Lacking that, Windows has an environment variable for TEMPDIR (iirc, need to confirm exact spelling) so we can relax the assumption on the C: drive. Otherwise, that is a point of failure, albeit uncommon.

…

On Mon, Mar 25, 2019, 4:54 PM Maksim Rakitin ***@***.***> wrote: ***@***.**** requested changes on this pull request. Looks good to me after @danielballan <https://github.com/danielballan>'s suggestions are implemented. Left one important suggestion regarding paths. ------------------------------ In event_model/tests/test_em.py <#59 (comment)>: > @@ -131,8 +146,14 @@ def test_bulk_events_to_event_page(): desc_bundle_baseline = run_bundle.compose_descriptor( data_keys={'motor': {'shape': [], 'dtype': 'number', 'source': '...'}}, name='baseline') + + if os.name == 'nt': + path_root = 'C:\tmp' To avoid it to be evaluated as "tab" (\t) ⬇️ Suggested change - path_root = 'C:\tmp' + path_root = 'C:\\tmp' Quick IPython demo: In [1]: path_root = 'C:\tmp' In [2]: path_root Out[2]: 'C:\tmp' In [3]: print(path_root) C: mp In [4]: path_root = 'C:\\tmp' In [5]: path_root Out[5]: 'C:\\tmp' In [6]: print(path_root) C:\tmp — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#59 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLKMGcmOowawNZ93aDHzHa_jMZvy8u2ks5vaPEmgaJpZM4b63Y8> .

mrakitin · 2019-03-25T17:10:17Z

Thanks for the feedback, @prjemian! I think the motivation here is to test with the Windows path semantics as it would be on a Windows collection machine with real paths, not just temporary placeholders. We had a similar problem in ophyd, bluesky/ophyd#703.

A general comment: it would be nice to have a template for the issues/PRs the same way we have it for Bluesky. I hope it will enforce stricter descriptions and motivations sections.

danielballan · 2019-03-25T17:32:58Z

I had the same thought as @prjemian but didn't bother raising it. I'm not sure I understand your point about testing a "real path". I think this would be a reasonable place to use pytest's tmp_path fixture which is how we handle this in suitcase-utils. It would remove a bunch of if blocks and also ensure that the sample files are cleaned up on test exit.

prjemian · 2019-03-25T17:34:18Z

+1 for tmp_path

…

On Mon, Mar 25, 2019, 6:33 PM Dan Allan ***@***.***> wrote: I had the same thought as @prjemian <https://github.com/prjemian> but didn't bother raising it. I'm not sure I understand your point about testing a "real path". I think this would be a reasonable place to use pytest's tmp_path fixture <https://docs.pytest.org/en/latest/tmpdir.html> which is how we handle this in suitcase-utils. It would remove a bunch of if blocks and also ensure that the sample files are cleaned up on test exit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#59 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLKMMGb2kMPF-IUTXooxADMRUIvQaLJks5vaQhKgaJpZM4b63Y8> .

danielballan · 2019-03-25T17:59:00Z

If you do go with tmp_path @gwbischof you will need to specify pytest >3.9 here: https://github.com/NSLS-II/event-model/blob/873a874b87b75b0a35dc70d9830bc3790f0c7211/requirements-dev.txt#L6

Travis comes with an older version of pytest pre-installed, so you need to specifically pin it to prompt pip to upgrade.

mrakitin · 2019-03-25T18:00:37Z

@danielballan, it seems the suggested tmp_path fixture will help here. My point was to have test cases, realistically describing different use cases. But I believe it can be obtained via the fixtures by providing different sets of parameters.

danielballan · 2019-03-25T22:57:45Z

OK, sounds good.

mrakitin

Looks good to me!

event_model/tests/test_em.py

gwbischof added 9 commits March 18, 2019 17:43

Updates for event pages without filled key, and added a test.

34c923b

Update to path semantics for windows

c16ae1e

Update to path semantics

81a83bc

Fixed a == that should of been =

9399fee

New unfilled test found that pack_event_page fails on events without …

3c46163

…filled key, made a fix for this

Return false if filled is not found

18b2ac9

Working on handling events without filled

466faba

Update to rount trip without filled test

bb91ca6

another no filled field change

81d7d6d

testing

53c2852

tacaswell reviewed Mar 19, 2019

View reviewed changes

event_model/__init__.py Outdated Show resolved Hide resolved

gwbischof added 4 commits March 19, 2019 14:24

Fixes to tests to get them working on windows

eb43c3c

flake8 fixes

49aa4e1

update to pack_event_page

ba63c2c

flaske8 update

3264dbb

gwbischof mentioned this pull request Mar 19, 2019

Mongo embedded bluesky/intake-bluesky#15

Merged

danielballan requested changes Mar 25, 2019

View reviewed changes

danielballan changed the title ~~Garrett~~ Fix path_sematnics and be robust against empty 'filled'. Mar 25, 2019

mrakitin requested changes Mar 25, 2019

View reviewed changes

mrakitin mentioned this pull request Mar 25, 2019

ENH: add templates for issues/PRs #62

Merged

mrakitin changed the title ~~Fix path_sematnics and be robust against empty 'filled'.~~ Fix path_semantics and be robust against empty 'filled'. Mar 25, 2019

made changes from review

b4728e3

update to use tmp_path pytest fixture

1137409

danielballan approved these changes Mar 27, 2019

View reviewed changes

danielballan self-requested a review March 27, 2019 19:09

danielballan approved these changes Mar 27, 2019

View reviewed changes

danielballan requested a review from mrakitin March 27, 2019 19:09

mrakitin approved these changes Mar 28, 2019

View reviewed changes

mrakitin reviewed Mar 28, 2019

View reviewed changes

event_model/tests/test_em.py Show resolved Hide resolved

mrakitin merged commit 6a6c579 into bluesky:master Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix path_semantics and be robust against empty 'filled'. #59

Fix path_semantics and be robust against empty 'filled'. #59

gwbischof commented Mar 18, 2019

gwbischof commented Mar 19, 2019

danielballan left a comment

danielballan Mar 25, 2019

gwbischof Mar 26, 2019

mrakitin left a comment

mrakitin Mar 25, 2019

gwbischof Mar 26, 2019

prjemian commented Mar 25, 2019 via email

mrakitin commented Mar 25, 2019

danielballan commented Mar 25, 2019

prjemian commented Mar 25, 2019 via email

danielballan commented Mar 25, 2019

mrakitin commented Mar 25, 2019

danielballan commented Mar 25, 2019

mrakitin left a comment

Fix path_semantics and be robust against empty 'filled'. #59

Fix path_semantics and be robust against empty 'filled'. #59

Conversation

gwbischof commented Mar 18, 2019

gwbischof commented Mar 19, 2019

danielballan left a comment

Choose a reason for hiding this comment

danielballan Mar 25, 2019

Choose a reason for hiding this comment

gwbischof Mar 26, 2019

Choose a reason for hiding this comment

mrakitin left a comment

Choose a reason for hiding this comment

mrakitin Mar 25, 2019

Choose a reason for hiding this comment

gwbischof Mar 26, 2019

Choose a reason for hiding this comment

prjemian commented Mar 25, 2019 via email

mrakitin commented Mar 25, 2019

danielballan commented Mar 25, 2019

prjemian commented Mar 25, 2019 via email

danielballan commented Mar 25, 2019

mrakitin commented Mar 25, 2019

danielballan commented Mar 25, 2019

mrakitin left a comment

Choose a reason for hiding this comment