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 support for URI backed TextResource #2760

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@filiphr

filiphr commented Aug 20, 2017

  • Adds the ability to create an URI backed TextResource via the TextResourceFactory
  • Caching of the URI loading is handled by the TextResourceLoader

Context

See #2663 for more context.

However, I have to stress something. I think that this change will not work for the main use case in the linked issue. The reason for that is the internal TextResource which I think is DownloadedUriTextResource returns null for getFile. The Checkstyle extension uses the api TextResource#asFile() for getting the XML. I am not sure where this needs to be addressed, whether as part of this PR, or a separate one.

Another approach would be to make sure that UriBackedTextResource (part of this PR) resorts to opening a reader, creating a temporary file and returning it if the internal TextResource does not return a file.

If you ask me I think that it needs to be fixed in the internal TextResource and the UriBackedTextResource is just a wrapper for the API.

Contributor Checklist

  • Review Contribution Guidelines
  • Sign Gradle CLA - I've made a small typo and would probably need to redo it. I've already sent an email to info@gradle.com
  • Link to Design Spec for changes that affect more than 1 public API (that is, not in an internal package) or updates to > 20 files - Not applicable according to #2663 (comment)
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation including proper use of @since and @Incubating annotations for all public APIs
  • Recognize contributor in release notes
* @param uri an uri
*
* @return a text resource backed by the given uri
* @since TODO

This comment has been minimized.

@filiphr

filiphr Aug 20, 2017

I was not sure which version should go here, I suppose 4.2. Once you let me know, I can update it without problem

@oehme

This comment has been minimized.

Member

oehme commented Aug 20, 2017

I'd overrride getFile in the underlying DownloadedUriTextResource. The field for the file is already there. Then your wrapper can become a generic one that can wrap any internal TextResource.

@filiphr

This comment has been minimized.

filiphr commented Aug 20, 2017

I'd overrride getFile in the underlying DownloadedUriTextResource. Then your wrapper can become a generic one that can wrap any internal TextResource.

That's great to hear. I was thinking in the same direction, but I was not sure whether it should be part of this PR 😄 . I'll update the PR with a revised DownloadedUriTextResource, and I'll even rename the UriBackedTextResource in something more general.

I'll see how I can make the InputProperties more general as well.

@filiphr

This comment has been minimized.

filiphr commented Aug 21, 2017

@oehme, I've tried to do what you proposed. However, that leads to some test failures on different location.

By looking at the documentation of the internal TextResource#getFile()

Returns a file that contains the same content as this resource, encoded using the charset specified by {@link #getCharset()}.
Not all resources are available as a file.
Note that this method may return null when {@link ResourceLocation#getFile()} returns non-null, when the contents are different.

I suppose that returning the downloaded file there is not correct. What would you suggest? Should I continue down this path, or should I just create a temp file from the contents of the TextResource and return it on the API?

@bmuschko

This comment has been minimized.

Contributor

bmuschko commented Aug 21, 2017

@filiphr I couldn't find your signed CLA. Did you already sign it? Did you use your own name?

@filiphr

This comment has been minimized.

filiphr commented Aug 21, 2017

@bmuschko Initially I used Cyrillic for the name, as I was not aware as that would be the name that you will need to search for 😄 . In any case I just signed another one. I used my name Filip Hrisafov and this GitHub profile

@bmuschko bmuschko added cla:yes and removed cla:no labels Aug 21, 2017

@bmuschko

This comment has been minimized.

Contributor

bmuschko commented Aug 21, 2017

@filiphr Great, thanks!

@filiphr

This comment has been minimized.

filiphr commented Aug 26, 2017

I just wanted to ask whether someone had the time to look at the PR and my comments as well. I would like to know how we should proceed with it. I can push what I have locally, but tests are failing with the suggestion of @oehme

@oehme

This comment has been minimized.

Member

oehme commented Sep 4, 2017

I've tried to do what you proposed. However, that leads to some test failures on different location.

Please push the change and let me know which tests fail and I'll take a look whether we can change the expectation or not.

filiphr added some commits Aug 20, 2017

Add support for URI backed TextResource
* Adds the ability to create an URI backed TextResource via the TextResourceFactory
* Caching of the URI loading is handled by the TextResourceLoader

Issue: #2663
@filiphr

This comment has been minimized.

filiphr commented Sep 4, 2017

@oehme I've pushed the change. There are cascading issues occurring. If I make the empty resource file return the passed file more issues occur...

@oehme oehme self-requested a review Sep 5, 2017

@oehme oehme self-assigned this Sep 5, 2017

@oehme

This comment has been minimized.

Member

oehme commented Sep 8, 2017

I took a look at the failing tests. They are asserting that a script using apply from: will report null as its source file, which breaks if we return the downloaded file. This probably means that the whole inheritance hierarchy here is a bit questionable, but I don't want to put that refactoring on you unless you are interested ;) So to keep it simple, let's go back to not changing that class and using a copy instead (like you already do for the charset mismatch case).

There is some duplication between FileCollectionBackedTextResource and the new wrapping text resource. It would be great if you could extract the common parts (handling of null File or charset mismatch).

Last but not least, we want to be lazy, i.e. don't download the file until it is really needed. For that you'll have to pass the TextResourceLoader into your wrapping text resource and only load the delegate when it is needed instead of loading it up-front and passing it to the constructor.

@filiphr

This comment has been minimized.

filiphr commented Sep 12, 2017

This probably means that the whole inheritance hierarchy here is a bit questionable, but I don't want to put that refactoring on you unless you are interested ;)

I think that this deserves its own issue. It will take a while if I need to do it. I think that I tried resolving it, but then other cases started failing 😄 .

There is some duplication between FileCollectionBackedTextResource and the new wrapping text resource. It would be great if you could extract the common parts (handling of null File or charset mismatch).

I'll see what I can do here. I could also extract some common class that we can delegate the work to.

Last but not least, we want to be lazy, i.e. don't download the file until it is really needed. For that you'll have to pass the TextResourceLoader into your wrapping text resource and only load the delegate when it is needed instead of loading it up-front and passing it to the constructor.

Of course. Will make it lazy.

@filiphr

This comment has been minimized.

filiphr commented Sep 12, 2017

@oehme I've applied the changes that you've mentioned. Actually, the implementation before was handling the case when the file was null 😄

@oehme oehme added this to the 4.8 RC1 milestone May 2, 2018

@oehme

This comment has been minimized.

Member

oehme commented May 3, 2018

Thank you and sorry for the long wait @filiphr. I have rebased your changes on the current master and opened #5243. This feature will be in 4.8

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