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

Fix issue #669 and add test case. #721

Merged
merged 13 commits into from
Oct 15, 2020

Conversation

willson-chen
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #721 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage   96.02%   96.07%   +0.04%     
==========================================
  Files          17       17              
  Lines        2668     2700      +32     
  Branches      310      310              
==========================================
+ Hits         2562     2594      +32     
  Misses         87       87              
  Partials       19       19              
Impacted Files Coverage Δ
jsonschema/cli.py 96.99% <100.00%> (+0.04%) ⬆️
jsonschema/tests/test_cli.py 99.19% <100.00%> (+0.11%) ⬆️

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 e48d56c...d106511. Read the comment docs.

@willson-chen
Copy link
Contributor Author

willson-chen commented Aug 13, 2020

Add test cases based on PR #680 and fix the base_uri layer problem.

The code before the rectification is as follows:

os.path.abspath(arguments["schema"])) 

Code after the rectification:

os.path.abspath(os.path.dirname(arguments["schema"]))

@Julian
Copy link
Member

Julian commented Aug 14, 2020

Thanks for trying to tackle this one as well. Have a look at the comment here from that PR -- specifically, I'd like to support this with a --base-uri flag instead, and if someone wants to use the working directory as that base URI, you'd specify --base-uri . essentially.

@willson-chen
Copy link
Contributor Author

@Julian Sorry, I ignored some of the details in the comment #680, now I have replaced -r with --base-uri.

jsonschema/cli.py Outdated Show resolved Hide resolved
@willson-chen
Copy link
Contributor Author

@Julian I have already pushed the repair part, comments are welcome.

jsonschema/cli.py Outdated Show resolved Hide resolved
jsonschema/cli.py Outdated Show resolved Hide resolved
@willson-chen
Copy link
Contributor Author

We can specify a remote or local path through --base-uri, where the local path can be a relative path or an absolute path. Through scheme = urllib.parse.urlsplit(arguments["base_uri"]).scheme, if the scheme is an empty string or a drive letter identifier from 'a' to 'z' is considered to be a local path, otherwise it is considered a remote path.
@Julian How about this solution?

@Julian
Copy link
Member

Julian commented Aug 30, 2020

I'd have to look more carefully myself but that doesn't sound right to me either I think.

I don't really want to change any default behaviors of things (what behavior folks get should match what behavior they get from normal other things).

Or, said differently, I don't want to maintain code that's trying to guess what kind of URI was provided as the base URI.

The base URI as provided needs to be suitable for use exactly as is, or be unambiguous.

Will leave a quick comment on the PR in the section that looks "suspicious" to me, let me know what you think.

jsonschema/cli.py Outdated Show resolved Hide resolved
@willson-chen
Copy link
Contributor Author

@Julian Modified to the following simple implementation, let users pass in the correct URI, for example --base-uri file:///c:/ --base-uri http://example, also supports --base-uri .
Is this implementation possible?

    if arguments["base_uri"] is None:
        resolver = None
    elif arguments["base_uri"] == ".":
        file_prefix = "file:///{}/" if "nt" == os.name else "file://{}/"
        resolver = RefResolver(
                    base_uri=file_prefix.format(os.getcwd()),
                    referrer=schema,
                )
    else:
        resolver = RefResolver(
                        base_uri=arguments["base_uri"],
                        referrer=schema,
                    )

jsonschema/cli.py Outdated Show resolved Hide resolved
@willson-chen
Copy link
Contributor Author

@Julian I have modified it according to your last review, could you please take a look again?

@willson-chen willson-chen force-pushed the fix_issue669 branch 3 times, most recently from 8d376e1 to 040b95a Compare September 16, 2020 08:42
@willson-chen
Copy link
Contributor Author

@Julian Sorry, I have been away for a while, but I never forget that there is still something unfinished. Is it possible to modify as follows? 1bfbb90

@Julian
Copy link
Member

Julian commented Sep 29, 2020

Hey! No worries (trust me I understand the busyness...)

From a quick look that definitely looks more like what I was expecting! Will have a closer look and leave any comments but think we may be close.

@willson-chen
Copy link
Contributor Author

@Julian Can this PR be merged? Ask a rather abrupt question, do you welcome new maintainers to take care of the community. I am willing to participate if needed.

* origin/master:
  Handle multipleOf overflow
  Don't fail when codecov.io flakes.
  pep517.build is dead or dying.
  Temporarily skip the failing tests for python-jsonschema#743.
  Sigh, ugly, but assrtRegexpMatches is deprecated.
  Squashed 'json/' changes from 21555a85..96742ba3
  Squashed 'json/' changes from fce9e9b3..21555a85
  Whoops, send coverage to codecov again.
  Be able to run coverage on all toxenvs.
  Remove outdated release notes.
  Use the longer form of the CLI option for clarity.
  Bump doc requirements (via pip-compile)
  Remove the Py2 coding declaration.
  Run pre-commit in CI.
  Remove docformatter, it's too unwieldy.
  Minor doc formatting.
  Sigh, this appears to be a regex, not exact match, so it was ignoring everything.
  Update pre-commit hooks.
@Julian
Copy link
Member

Julian commented Oct 15, 2020

Apologies for the delay again here...

There were a few things I wanted to write up comments on which I didn't get a chance to, so I just fixed them in a few commits.

I think this should be good to merge now as soon as CI passes.

On your offer, 100%! I definitely could use more help. If you see pull requests from other folks that aren't yet fully mergeable, or waiting themselves for review, or issues that could use triaging, etc. it would be super super appreciated.

@Julian Julian merged commit 5911287 into python-jsonschema:master Oct 15, 2020
@RomainTT
Copy link
Contributor

A year passed and I did not thank you both yet !

Thank you @willson-chen and @Julian for finishing this work! That is a very useful feature.

Julian added a commit that referenced this pull request Apr 30, 2024
54f3784a8 Merge pull request #731 from MeastroZI/main
ff29264c2 Merge pull request #741 from harrel56/chore/tabs-to-spaces
9f39cf731 use spaces instead of tabs
2f3b5f7ab Corrected replaced unevaluated with additoinalProperties
fa9224d7e Merge pull request #732 from MeastroZI/main2
83bedd5c8 Changing descriptions
49f73429c fixing tests
e6d6a0816 adding more test cases
7e6c9be6d changing descriptions
959aca926 shifting test
605d7d786 Update propertyDependencies.json : test must be tests
deb82824b test for dependentSchema and propertyDependencies with unevaluatedProperties and additionalProperties
ea4851242 Merge branch 'json-schema-org:main' into main
64a3e7b37 Merge pull request #721 from json-schema-org/gregsdennis/dynamicref-skips-resources
b9f14e64c Fix $schema in new new test
3d5048e83 Merge pull request #733 from Era-cell/main
2480edbae Update additionalProperties.json formatting it
6aa79c0b2 Update additionalProperties.json formatting it
3e0139a54 Update tests/draft-next/additionalProperties.json
616240b06 Update tests/draft-next/additionalProperties.json
c5f3e4eaa Update tests/draft2020-12/propertyNames.json
964efb8e6 propertyNames doesn't affect additionalProperties, tests exist already for unevaluatedProps
f08b884cf Cases go under additional and unevaluated Properties
99864ff66 added tests for propertyNames with additionalProperties/unevaluatedProperties, also with specification property
3b5782b65 Update ref.json : changing $Ids
546b3561a test for $ref with $recursiveAnchor
57617f254 Merge pull request #726 from Era-cell/main
51fc69cd7 meta data and property names constraints added, additional Items: string
9b169bed8 specification takes array of objects having section and quote
1362a8cce Pattern for para corrected
340116ecf Schema of specification in much structured
003ac0211 Test-schema including sub-schema for scpecification
50a20280b adding specification enhancement for additionalProperties
604f5f99b Drop tests of `$id` and `$anchor` that just test values against meta-schema `pattern` for those properties
9cd64ec94 come on man, save all the files
f494440e3 use unique $id in optional tests, too
468453b0f use unique $id
9ec6d17e7 fix copy/paste error
b284f4232 add tests for $dynamicRef skipping over resources
bf0360f4b add $recursiveAnchor to 2019-09 meta-schemas
0519d1f0e add $dynamicAnchor to meta-schemas

git-subtree-dir: json
git-subtree-split: 54f3784a8c6926d8e91ed43267950a07efc34086
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