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

feat: Handle absolute paths in XML config files (xml2json / readxml) #1909

Merged
merged 14 commits into from
Aug 11, 2022

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Jul 1, 2022

Pull Request Description

  • support XML configs with absolute paths by being able to prune the paths out/replace them to point elsewhere
  • typehint the entire thing and fix some minor holes

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add support for XML configurations with absolute paths by pruning the
paths out and renaming paths on-the-fly in readxml.
* Add -v/--mounts command line arguments for xml2json with similar behavior
to the Docker equivalent to support this feature.
* Add tests and test XML files with absolute paths.

@matthewfeickert matthewfeickert added feat/enhancement New feature or request CLI Affects the CLI API labels Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #1909 (124ff59) into master (1ed7003) will decrease coverage by 0.03%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #1909      +/-   ##
==========================================
- Coverage   98.22%   98.19%   -0.04%     
==========================================
  Files          68       68              
  Lines        4334     4365      +31     
  Branches      730      734       +4     
==========================================
+ Hits         4257     4286      +29     
- Misses         45       46       +1     
- Partials       32       33       +1     
Flag Coverage Δ
contrib 26.25% <9.09%> (-0.10%) ⬇️
doctest 60.50% <52.27%> (-0.11%) ⬇️
unittests-3.10 96.08% <95.45%> (-0.02%) ⬇️
unittests-3.7 96.06% <95.34%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/readxml.py 95.21% <93.33%> (-0.67%) ⬇️
src/pyhf/cli/rootio.py 92.59% <100.00%> (+0.28%) ⬆️
src/pyhf/utils.py 95.91% <100.00%> (+1.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matthewfeickert
Copy link
Member

From @kratsg's testing so far it looks like this will be able to address Issue #1882. 👍

@matthewfeickert matthewfeickert mentioned this pull request Jul 1, 2022
1 task
@kratsg kratsg marked this pull request as ready for review August 9, 2022 22:20
@kratsg kratsg force-pushed the feat/xml2jsonAbsPath branch from 74e543e to 75cc278 Compare August 10, 2022 13:41
src/pyhf/typing.py Outdated Show resolved Hide resolved
@kratsg kratsg force-pushed the feat/xml2jsonAbsPath branch from 32fb70f to 0c29f3e Compare August 10, 2022 21:40
@kratsg kratsg force-pushed the feat/xml2jsonAbsPath branch from 0c29f3e to 2c3c482 Compare August 10, 2022 21:53
@kratsg kratsg requested a review from matthewfeickert August 10, 2022 21:56
@kratsg kratsg closed this Aug 10, 2022
@kratsg kratsg reopened this Aug 10, 2022
@kratsg
Copy link
Contributor Author

kratsg commented Aug 10, 2022

@matthewfeickert this is ready, but codecov is broken for this one (some glitch on their side of things) probably related to my rebasing to clean things up.

@matthewfeickert matthewfeickert added the tests pytest label Aug 11, 2022
@matthewfeickert
Copy link
Member

Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking super good.

I have a few questions and suggestions, but nothing major, just things that we could decide if we want to fixup now as they're getting added for the first time.

src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/readxml.py Outdated Show resolved Hide resolved
src/pyhf/utils.py Show resolved Hide resolved
kratsg and others added 4 commits August 10, 2022 23:54
@kratsg kratsg requested a review from matthewfeickert August 11, 2022 04:03
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

LGTM. 👍 Thanks @kratsg!

@kratsg kratsg merged commit 97664bc into master Aug 11, 2022
@kratsg kratsg deleted the feat/xml2jsonAbsPath branch August 11, 2022 04:46
matthewfeickert added a commit that referenced this pull request Aug 26, 2022
* Update lower bound of the supported click versions to v8.0.0
as it is required to support behavior in PR #1909.
* Update tests/constraints.txt to use click==8.0.0.
matthewfeickert added a commit that referenced this pull request Aug 26, 2022
* Update lower bound of the supported click versions to v8.0.0
as it is required to support behavior in PR #1909.
* Update tests/constraints.txt to use click==8.0.0.
kratsg pushed a commit that referenced this pull request Aug 26, 2022
* Update lower bound of the supported click versions to v8.0.0 as it
is required to support behavior in PR #1909 due to click v7.x not
setting the type correctly for path type arguments.
   - c.f. #1957 (comment)
* Update tests/constraints.txt to use click==8.0.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Affects the CLI API feat/enhancement New feature or request tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants