Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#350 code changes, docs will need changes, too, also test code #351

Merged
merged 9 commits into from
Jun 25, 2018
Merged

#350 code changes, docs will need changes, too, also test code #351

merged 9 commits into from
Jun 25, 2018

Conversation

prjemian
Copy link
Collaborator

Before this is merged, will need docs (both in argparse setup and sphinx) and unit tests.

python ./main.py -m "AA=aa,BB=bb, CC=c{c}, DD=d[a<b}:_"

@AppVeyorBot
Copy link

Build pydm 1.0.166 completed (commit bf0f07c7cb by @prjemian)

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #351 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   42.88%   42.88%   +<.01%     
==========================================
  Files          69       69              
  Lines        6212     6198      -14     
==========================================
- Hits         2664     2658       -6     
+ Misses       3548     3540       -8
Impacted Files Coverage Δ
pydm/PyQt/QtSvg.py 100% <ø> (+16.66%) ⬆️
pydm/PyQt/Qt.py 100% <ø> (+16.66%) ⬆️
pydm/PyQt/uic.py 100% <ø> (+16.66%) ⬆️
pydm/PyQt/QtDesigner.py 100% <ø> (+16.66%) ⬆️
pydm/widgets/qtplugin_base.py 55.55% <ø> (ø) ⬆️
pydm/PyQt/qtlib.py 55% <0%> (ø) ⬆️
pydm/PyQt/QtCore.py 100% <100%> (+33.33%) ⬆️
pydm/PyQt/QtGui.py 100% <100%> (+12.5%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e33844...cc92941. Read the comment docs.

@AppVeyorBot
Copy link

Build pydm 1.0.167 completed (commit 7f57282a02 by @prjemian)

@AppVeyorBot
Copy link

Build pydm 1.0.168 completed (commit 6960d1c810 by @prjemian)

@mattgibbs
Copy link
Collaborator

Pete, this seems great! One tricky thing to note:

You can pass macros to python-based displays too, and those displays just get a dictionary that they can handle however they want. Because the JSON syntax allows you to differentiate between string values and numeric values ({"my_num": 5} vs {"my_num":"5"}), using your newer, less-verbose format might confuse some displays that are written to expect a numeric value, but are handed a string.

I don't think that is a deal-breaker at all. It is a relatively easy fix for people writing displays to ensure they convert to a float or int or whatever before using macro arguments. We probably should have a note in the documentation explaining that if you use JSON syntax, you need to think about numeric vs string, and if you use the concise syntax, you can only ever get strings.

@prjemian
Copy link
Collaborator Author

It's a good thing to note. I was thinking about that as well. Without the JSON coding, all command line parameters are passed as strings. The other display managers handle the macros as strings. Are there cases for PyDM that require these values to be processed numerically? A note in the documentation should suffice.

@hhslepicka
Copy link
Contributor

I think that we should give notice to developers and screens should expect to receive macros as strings and parse as they need it. Since it comes from the command line they should deal with that along the line and not expect the value to be on the type but ensure with a cast.
I think that a note in the documentation is good enough to deal with this case.

@AppVeyorBot
Copy link

Build pydm 1.0.179 completed (commit d48a0b77b8 by @mattgibbs)

@mattgibbs mattgibbs merged commit b5c2621 into slaclab:master Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants