search

pyepics / newportxps

python support code for NewportXPS drivers
BSD 2-Clause "Simplified" License
21 stars 18 forks source link

Clean up output parsing - use common function and remove eval #21

Closed dennisjlee closed 3 months ago

dennisjlee commented 4 months ago

@newville let me know how you feel about this, I'm very open to feedback!

As I started using the library I noticed that there was a lot of repetitive parsing code in this file, and it all relied on eval which is less secure and performant. I tried to rewrite in a different way to emphasize safety and performance.

See microbenchmark here:

image
newville commented 4 months ago

@dennisjlee Yeah, this file is derived from the original code provided by Newport. It was always a bit ugly and had to be altered to support both Py2 and Py3 for a while. It has not aged well. I think it would benefit from cleanup and refactoring. Like, using f-strings would be very helpful. We don't have a good automated test suite or process, but I think that should not necessarily prevent refactoring.

dennisjlee commented 4 months ago

Cool thanks for the context @newville. I used f-strings in places that I touched and also made an effort at adding type annotations, but I could definitely do more in the future. I saw in the pyproject.toml that this project supports Python 3.8+ so I used that when developing and made sure to use type annotations that are compatible with 3.8.

newville commented 3 months ago

@dennisjlee I am in a better place to look at this, so I am going to merge this (and then test it locally a bunch). I'm hoping to tag a release of 1.0.0 soon-ish.

dennisjlee commented 3 months ago

@newville thank you very much! Let me know if you have any feedback or find any bugs, happy to follow up.

I haven't had much time to look more at this codebase lately but one thing I've been pondering in the back of my head is how to make these functions in XPS_C8_drivers.py have correct return types annotated (automatically if possible). If I have more time I'll see if I can tinker with that further.