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

Guid-helpers: Use ast instead of regex #2700

Merged
merged 20 commits into from
Feb 17, 2021

Conversation

Dominik-Vogel
Copy link
Contributor

The regular expression used to read guids from a string represantation of a list/tuple of guids was flawed and managed to sneak around the test, by working on 0, 1, and 2 guids.
In this PR I am using the ast.parse instead, which makes it a lot easier and error proof.
Thanks @astafan8 !

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! :)

qcodes/dataset/guid_helpers.py Outdated Show resolved Hide resolved
qcodes/dataset/guid_helpers.py Outdated Show resolved Hide resolved
qcodes/dataset/guid_helpers.py Outdated Show resolved Hide resolved
@astafan8 astafan8 added this to the 0.23.0 milestone Feb 11, 2021
@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #2700 (7c71a4f) into master (28ea0dc) will increase coverage by 0.00%.
The diff coverage is 80.76%.

@@           Coverage Diff           @@
##           master    #2700   +/-   ##
=======================================
  Coverage   63.83%   63.84%           
=======================================
  Files         198      198           
  Lines       26470    26487   +17     
=======================================
+ Hits        16898    16911   +13     
- Misses       9572     9576    +4     

@astafan8 astafan8 merged commit aec646d into microsoft:master Feb 17, 2021
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.

None yet

3 participants