Skip to content

Turn these classes to public #672

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

Closed
wants to merge 2 commits into from

Conversation

rogerhu
Copy link
Contributor

@rogerhu rogerhu commented May 23, 2017

Just tried to change Parse Live Query package name but there actually classes that need to be made public in order to make this possible. For now, revert to not using BuildConfig in the Parse SDK Android.

/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/Subscription.java:18: error: State is not public in ParseQuery; cannot be accessed from outside package
    private final ParseQuery.State<T> state;
                            ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/Subscription.java:71: error: State is not public in ParseQuery; cannot be accessed from outside package
    /* package */ ParseQuery.State<T> getQueryState() {
                            ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/ParseLiveQueryClientImpl.java:7: error: ParseDecoder is not public in com.parse; cannot be accessed from outside package
import com.parse.ParseDecoder;
                ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/ParseLiveQueryClientImpl.java:9: error: ParsePlugins is not public in com.parse; cannot be accessed from outside package
import com.parse.ParsePlugins;
                ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/ParseLiveQueryClientImpl.java:11: error: ParseRESTCommand is not public in com.parse; cannot be accessed from outside package
import com.parse.ParseRESTCommand;
                ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/SubscribeClientOperation.java:5: error: PointerEncoder is not public in com.parse; cannot be accessed from outside package
import com.parse.PointerEncoder;
                ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/Su`bscribeClientOperation.java:14: error: State is not public in ParseQuery; cannot be accessed from outside package
    private final ParseQuery.State<T> state;
                            ^
/Users/rhu/projects/ParseLiveQuery-Android/ParseLiveQuery/src/main/java/com/parse/livequery/SubscribeClientOperation.java:17: error: State is not public in ParseQuery; cannot be accessed from outside package
    /* package */ SubscribeClientOperation(int requestId, ParseQuery.State<T> state, String sessionToken) {
                                                                    ^
``

@Jawnnypoo
Copy link
Member

Does BuildConfig not get created if you don't reference it? I am thinking it still might get generated anyways.

@rogerhu
Copy link
Contributor Author

rogerhu commented May 23, 2017

Yeah I just noticed BuildConfig is still there.

I think this is related to going to AAR only packages.

@codecov
Copy link

codecov bot commented May 23, 2017

Codecov Report

Merging #672 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #672      +/-   ##
============================================
- Coverage     52.88%   52.86%   -0.02%     
  Complexity     1676     1676              
============================================
  Files           131      131              
  Lines         10150    10150              
  Branches       1408     1408              
============================================
- Hits           5368     5366       -2     
- Misses         4340     4341       +1     
- Partials        442      443       +1
Impacted Files Coverage Δ Complexity Δ
Parse/src/main/java/com/parse/ParseQuery.java 77.77% <ø> (ø) 83 <0> (ø) ⬇️
...arse/src/main/java/com/parse/ParseRESTCommand.java 82.88% <ø> (ø) 52 <0> (ø) ⬇️
Parse/src/main/java/com/parse/ParsePlugins.java 42.85% <ø> (ø) 12 <0> (ø) ⬇️
Parse/src/main/java/com/parse/ParseDecoder.java 96.36% <ø> (ø) 21 <0> (ø) ⬇️
Parse/src/main/java/com/parse/PointerEncoder.java 100% <100%> (ø) 5 <1> (ø) ⬇️
...se/src/main/java/com/parse/ParseKeyValueCache.java 45% <0%> (-2%) 16% <0%> (ø)

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 ff7c712...c7944a3. Read the comment docs.

@rogerhu rogerhu changed the title Build config Turn these classes to public May 23, 2017
@rogerhu
Copy link
Contributor Author

rogerhu commented May 23, 2017

Changed the classes needed for ParseLiveQuery to be public...thoguhts on this?

@Jawnnypoo
Copy link
Member

Sounds good, I think it would be good to document the classes to let others know they are not truly meant for public use, so that we can change them without having to worry about compatibility.

Something like This class is public in order to allow for other Parse SDKs to work. Please note that the API of this class can change with any release or something like that

@natario1
Copy link
Contributor

Is there any other option? Some build magic to solve the BuildConfig issue? Just my opinion, but I would rather move the whole live query to this repo at this point, if it really needs these internals.

@rogerhu
Copy link
Contributor Author

rogerhu commented May 23, 2017

I don't see a way in the Gradle plugin to rename BuildConfig to avoid conflicts. That would be my first choice...

@Jawnnypoo
Copy link
Member

I don't think its a great idea to pull in the live query stuff into this SDK since many might not use it and would just be confused by it.

I think there are many reasons besides duplicated BuildConfig as to why they should not share a package name. I think that is honestly the best solution, thought it would involve a major release in Live Query since it would break compatibility.

You can try setting an applicationId in the build.gradle to a different package name and see if that generates a different BuildConfig in Live Query. But in the end, my vote goes toward once and for all just changing the package name of Live Query to prevent other future issues.

@natario1
Copy link
Contributor

Yeah, well my comment was more like “This looks so bad to me that I would rather...”. But I don’t see any solution either. I also agree that the package should be changed, or live query pulled here.

Copy link
Contributor

@hermanliang hermanliang left a comment

Choose a reason for hiding this comment

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

I think the simplest way is changing the LiveQuery package name (ex. com.parse.livequery) in AndroidManifest.xml and keeping the same file structure.

Therefore, BuildConfig will be here
/com/parse/livequery/BuildConfig.class

@hermanliang
Copy link
Contributor

Just submit a PR in ParseLiveQuery
parse-community/ParseLiveQuery-Android#64

@Jawnnypoo
Copy link
Member

Ah that's a good point, I think that would work great

@natario1
Copy link
Contributor

So for the future, should we start working on allowing LiveQuery to use new, meaningful public APIs and later change the file structure as well? By this I mean create new public APIs here for what LQ needs. I am not confident with LQ, but looking at the first comment in this PR it requires:

  • ParsePlugins to get application Id and client key: we can expose these in Parse class
  • ParseRestCommand to get server url: expose in Parse class
  • ParseDecoder + PointerEncoder: that’s JSON encoding/decoding. Depending on what they are used for (ParseObjects or what?) we can offer static methods in this repo that might be useful for any user
  • ParseQuery.State: I guess that it must be made public. It is accessed with query.getBuilder().build() which looks dirty, so we could add something like public ParseQuery.State freeze() { return builder.build(); } to get a immutable state from a ParseQuery. If we make this state Parcelable, it might have a sense for the developer as well.

Things will be probably worse because the first comment here only lists classes to be made public, not methods.

@flovilmart
Copy link
Contributor

We have pretty much the same issue with the iOS, most notably for serialization of queries and objects.

@@ -297,7 +297,7 @@ private static void throwIfLDSEnabled(boolean enabled) {
}
}

/* package */ static class State<T extends ParseObject> {
/* package */ public static class State<T extends ParseObject> {
Copy link
Member

Choose a reason for hiding this comment

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

@rogerhu Can we remove the `/* package */ lines since they would be irrelevant since the class is public?

@rogerhu rogerhu closed this Jun 21, 2017
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.

5 participants