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

Deprecate Fragments #2886

Closed
wants to merge 2 commits into from
Closed

Deprecate Fragments #2886

wants to merge 2 commits into from

Conversation

jaredsburrows
Copy link
Contributor

Closes #2883

@sjudd

@sjudd
Copy link
Collaborator

sjudd commented Feb 9, 2018

Thanks! Would you mind adding an @deprecated javadoc to the public methods you deprecated with an explanation of what users should use instead? Might even be helpful to link to the post you mentioned in the issue that indicates that non-support fragments will be deprecated.

@sjudd
Copy link
Collaborator

sjudd commented Feb 9, 2018

I think you're also missing suppressions to avoid spurious deprecation warnings in Glides code. It looks like I inadvertently removed -Werror. I'll re-enable that so you'll see exactly where the suppressions are needed.

@jaredsburrows
Copy link
Contributor Author

jaredsburrows commented Feb 9, 2018 via email

sjudd added a commit that referenced this pull request Feb 9, 2018
@jaredsburrows
Copy link
Contributor Author

@sjudd How do I append @deprecated Use method {@link #with(Fragment)} instead.? It automatically adds @Deprecated but not the comment.

@sjudd
Copy link
Collaborator

sjudd commented Feb 10, 2018

/**
  * @deprecated use xyz
  */
@Deprecated
public RequestManager with(...) {
...
}

Is that what you're asking?

Also sorry for the build breakage, I believe I've fixed it, but if not I'll get it done tonight.

@jaredsburrows
Copy link
Contributor Author

@sjudd I tried locally but the GlideApp.java auto generated for the test removes the @deprecated use xyz.

@sjudd
Copy link
Collaborator

sjudd commented Feb 11, 2018

Ah, to fix the annotation processor tests, see http://bumptech.github.io/glide/dev/contributing.html#annotation-processor-tests

@jaredsburrows
Copy link
Contributor Author

jaredsburrows commented Feb 11, 2018

@sjudd Every time I run gradlew :annotation:compiler:test:test or gradlew :annotation:compiler:test:regenerateTestResources it removes @deprecated Use method {@link #with(Fragment)} instead. and then the tests fail.

@sjudd
Copy link
Collaborator

sjudd commented Feb 11, 2018

Are you modifying the generated files? Or the source? You shouldn't be changing GlideRequests, or GlideRequest directly.

@jaredsburrows
Copy link
Contributor Author

@sjudd I am not. I am trying to change GlideApp. See here: https://github.com/bumptech/glide/pull/2886/files#diff-f20041aac0f70f043e9b2b81facbeb37R121.

When the tests run, it removes the @deprecated that you want.

@sjudd
Copy link
Collaborator

sjudd commented Feb 11, 2018

Removes it from where?

If you just run the regenerate command I linked above, it should update the existing test resource files including the one you linked to match the actual output of the annotation processor.

@jaredsburrows
Copy link
Contributor Author

@sjudd That is what I am saying. I have manually edited GlideApp but it does not get added to the test resources when running gradlew :annotation:compiler:test:regenerateTestResources.

@sjudd
Copy link
Collaborator

sjudd commented Feb 11, 2018

GlideApp is a generated class, like GlideRequests/GlideRequest, you shouldn't be modifying it directly. The generated one is based on Glide.java and the test resource files will be updated when you run that gradle command.

@jaredsburrows
Copy link
Contributor Author

Sure. Look at Glide.java that I edited. When I run gradlew :annotation:compiler:test:regenerateTestResources it removes @deprecated Use method {@link #with(Fragment)} instead..

See:

screen shot 2018-02-11 at 10 08 15 am

To follow your instructions, I went ahead and just pushed up the changes.

@sjudd
Copy link
Collaborator

sjudd commented Feb 11, 2018

Yeah that's expected. If you want to modify the generated javadoc, you'd need modify the annotation processor directly. Probably just @see is sufficient.

@jaredsburrows
Copy link
Contributor Author

@sjudd This next error is a checkstyle error. I did not add any code, only comments. I am not sure what you want me to do here.

@sjudd
Copy link
Collaborator

sjudd commented Feb 12, 2018

The error message is printed in travis:

/home/travis/build/bumptech/glide/integration/recyclerview/src/main/java/com/bumptech/glide/integration/recyclerview/RecyclerViewPreloader.java
╔════════════╤══════════════════════════════════════════════════════════════╤══════════╤══════╤═════════════════════════════════════════════════╗
║ Reporter   │ Rule                                                         │ Severity │ Line │ Message                                         ║
╠════════════╪══════════════════════════════════════════════════════════════╪══════════╪══════╪═════════════════════════════════════════════════╣
║ Checkstyle │ com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck │ ERROR    │ 66   │ Line is longer than 100 characters (found 135). ║
╚════════════╧══════════════════════════════════════════════════════════════╧══════════╧══════╧═════════════════════════════════════════════════╝

@jaredsburrows
Copy link
Contributor Author

@sjudd Sorry I was not feeling well. Should be all green now!

@sjudd
Copy link
Collaborator

sjudd commented Feb 12, 2018

Merged: 29d481c and 9922972.

Thanks!

@sjudd sjudd closed this Feb 12, 2018
@jaredsburrows jaredsburrows deleted the jb/deprecate-fragment branch February 12, 2018 17:52
sjudd added a commit to sjudd/glide that referenced this pull request Mar 6, 2018
-------------
Include the debug aar in release artifacts for Android projects.

We removed the release variant a while ago to speed up the build, which
has the side affect of removing the release aar from artifacts. Since
we expect the debug and release variants to be identical (hence why
we disabled the release variant), it should be safe to just use the
debug aar instead. We will have to specify it explicitly since android’s
rules unsurprisingly only add the release variant by default.

-------------
Bump version to 4.6.0

-------------
Update readme to 4.6.0

Also removes the old v4 dependency from maven deps, I don’t think it’s
necessary.

-------------
Change update_javadocs to use debugJavadocJar instead of release.

We’ve disabled the release variant.

-------------
Bump version to 4.7.0-SNAPSHOT

-------------
Add POM dependencies explicitly.

Fixes bumptech#2863.

-------------
Bump version to 4.6.1

-------------
Update readme to 4.6.1

-------------
Fix param mistake (bumptech#2873)

-------------
Update SimpleTarget javadoc to match v4 API.

-------------
Add javadoc for RequestOptions.apply/RequestBuilder.apply.

Related to bumptech#2858.

-------------
Add support for Uri data uris.

Previously we only supported data uris if they were provided as Strings.

Fixes bumptech#556.

-------------
Make GlideBuilder.build package private.

It shouldn’t have been made visible and can’t
safely be used directly outside of the library.

Fixes bumptech#2885

-------------
Handle notifications in MultiFetcher after cancellation.

We can’t guarantee that every fetcher implementation will strictly avoid
notifying after they’ve been cancelled.

This also improves the behavior in VolleyStreamFetcher so that it attempts to avoid notifying after cancel, although it still doesn’t make
any strict guarantee.

Fixes bumptech#2879

-------------
Re-enable -Werror for java compilation.

Related to bumptech#2886.

-------------
Fix a deprecation warning in DataUriTest.

-------------
gradle 4.5.1

-------------
deprecate fragments

-------------
add @deprecated to javadoc and suppress deprecations in code

-------------
Created by MOE: https://github.com/google/moe

MOE_MIGRATED_REVID=99229725401d5777e059da7b6331134bf73fbcdf

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=185535564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants