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

new method for getting the content in the file FileGetContents #735

Closed

Conversation

marineusde
Copy link

That do some problems in unit tests, cause you cant mock or fake the schema files. The content getter is separated now to override it, if someone use test frameworks.

@DannyvdSluijs
Copy link
Collaborator

Hi @marineusde

Thanks for your contribution. I do have some questions regarding this PR.

  1. Can you explain this PR a bit better? What is it solving, how is it solving it. Some examples would greatly help in understanding these changes.

  2. I'm curious why you're reverting a fix made in Addresses Content-Type mismatch on HTTP 301 Moved Permanently with FileGetContents #709 without addressing the why?

@marineusde
Copy link
Author

Hi @marineusde

Thanks for your contribution. I do have some questions regarding this PR.

1. Can you explain this PR a bit better? What is it solving, how is it solving it.  Some examples would greatly help in understanding these changes.

2. I'm curious why you're reverting a fix made in [Addresses Content-Type mismatch on HTTP 301 Moved Permanently with FileGetContents #709](https://github.com/jsonrainbow/json-schema/pull/709)  without addressing the why?

Sorry! I explain it in the issue, dont know why github doesnt linked it here:

#734

I dont want to revert anything, I just want to split the code for getting the content in a different method.

@DannyvdSluijs
Copy link
Collaborator

As mentioned in #734 I highly recommend implementing your own version of the required interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants