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

Add pv argument to get_all_pvs that allows a glob pattern. #53

Merged
merged 5 commits into from
Jun 30, 2021
Merged

Conversation

willrogers
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Nov 20, 2018

Pull Request Test Coverage Report for Build 368

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 97.739%

Files with Coverage Reduction New Missed Lines %
aa/utils.py 2 90.91%
Totals Coverage Status
Change from base Build 364: 0.01%
Covered Lines: 562
Relevant Lines: 575

💛 - Coveralls

@aawdls
Copy link
Contributor

aawdls commented Jun 29, 2021

@willrogers Any reason why this was not merged?

@willrogers
Copy link
Collaborator Author

I don't remember. Does it look ok to you?

@aawdls
Copy link
Contributor

aawdls commented Jun 29, 2021

It looks sensible enough. I will test it, and assuming it works, rebase and merge.

@aawdls
Copy link
Contributor

aawdls commented Jun 29, 2021

OK I think I know the reason why you postponed this. I get the same test failure that you do.

Adding the extra argument here makes this the only use with more than one URL parameter. Because of the non-deterministic ordering in dictionaries, two successive calls to AaRestClient._construct_url output the parameters in different orders. This causes the test to fail.

My preferred way to fix this would be to change _construct_url to accept an OrderedDict for the URL parameters, instead of using **kwargs.

I guess at the time you questioned whether this was worth it. I think it probably is so I'll go ahead and do it for neatness' sake.

@willrogers
Copy link
Collaborator Author

That sounds right. Deterministic URLs make sense I think.

Addition of `pv` argument to `RestClient.get_all_pvs` showed up
a problem in `_construct_url` that ordering of URL parameters
was not repeatable because they are got from the kwargs as a dict.

We get around this by sorting by key and storing as a list of
tuples of (key, value). We assume that no-one cares what the
order of URL parameters is, as long as it is deterministic.
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #53 (2561437) into master (6d0d4cc) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   97.60%   97.62%   +0.02%     
==========================================
  Files          10       10              
  Lines         625      632       +7     
==========================================
+ Hits          610      617       +7     
  Misses         15       15              
Impacted Files Coverage Δ
aa/rest.py 98.21% <100.00%> (+0.13%) ⬆️
aa/utils.py 90.38% <100.00%> (+0.58%) ⬆️

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 6d0d4cc...2561437. Read the comment docs.

aa/utils.py Outdated Show resolved Hide resolved
@willrogers
Copy link
Collaborator Author

Thanks, looks good.

@aawdls aawdls merged commit 51e606b into master Jun 30, 2021
@willrogers willrogers deleted the pv-search branch June 30, 2021 15:11
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