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

Consistency in return values #15

Open
IzzySoft opened this issue Dec 9, 2020 · 15 comments · Fixed by #16
Open

Consistency in return values #15

IzzySoft opened this issue Dec 9, 2020 · 15 comments · Fixed by #16

Comments

@IzzySoft
Copy link
Contributor

IzzySoft commented Dec 9, 2020

During my recent implementations and rewrites, I've introduced some "error reporting" to make it easier for the caller to figure if and what might have gone wrong. On error, several methods now return an array like

[success:0, message:"reason"]

But not all of them – for example, most of the search/browse methods supposed to return a simple array of package names don't have this. There would be two options to reach (a sort of) consistency:

  • returning [success:0, message:reason] instead (and if so, include success:1 with a "good result) also for those methods that currently do not, moving the "real results" into a "sub-array" (which then, on error, could be empty or just not present at all)
  • simply returning an empty array, and have a getLastError() method for obtaining the reason for all search/browse methods while keeping the current behavior for the others.

I'd prefer the first approach (so it's completely consistent) with the "empty real result". This combines the best of two worlds, e.g.

$apps = $google->parseWhatever();
if ( empty($apps['data']) ) { // this could simply mean nothing found matching the criteria
  if ( $apps['success'] ) { log('nothing found'); }
  else { log ('ERROR occured: '.$apps['message']); }
} else { // do something with the data

I'd even go as far as to always include the message key, just leaving its value empty on success. That would make things most consistent.

What's your stance? If you agree, I'd go over the entire class another time and make it consistent:

  • success:0 only on errors – not generally on "no results" for a user-specified search (I vaguely remember I accidentally made it such in one case). message then holds the reason (e.g. the HTTP response, or parse error when an expected pattern didn't match, etc)
  • success:1 on success. message then is present but usually empty, but might eg hold a hint on why the result set is empty (like "no hits").

If we want to fix it, we want to do that as early as possible – before there are users whose code would otherwise break on some update.

@BaseMax
Copy link
Owner

BaseMax commented Dec 9, 2020

Thanks, Yes, I did see some error message/warning.

We have another repository similar to this, We can take action these repos after this.
So no worry to apply minimal apply to structure. It's not much problem.
So can.

@BaseMax
Copy link
Owner

BaseMax commented Dec 9, 2020

The difference between this repository and another repository is that the other one only focuses on the database.
https://github.com/BaseMax/GooglePlayDatabaseMirror

So I think it's best to use the final version of the library in the GooglePlayDatabaseMirror repository.
Comment?

@IzzySoft
Copy link
Contributor Author

IzzySoft commented Dec 9, 2020

I'd prefer to finish it here. Using the latest version from the other project would mean starting all over again. Here I'm almost done now – what I described in this issue are the last remaining steps.

After reaching consistency, let's increase the version number here and declare it "RC". Next thing needed is what your TODO in the Readme says: "Unit test". In parallel, a set of "calls" can be set up for those methods available in both libraries, and the output compared so you can see what must be adjusted so your cron job won't break. Then the library from here can be taken to a branch of the other project (maybe as git submodule, to not have multiple copies to keep in sync), and adjustments be made there.

Funny coincidence: that other project has quite some parallels to what I want to use this library for. There are 14k5 apps in my database currently 😆 With the difference that my apps are from multiple sources (Play, F-Droid, my repo, Aptoide, Xposed), my cron job runs 4 times a day and merges those sources – each time a subset of the database to not hit some wall one day). For 2-3 days now, GooglePlayWebServiceAPI covers the permission part for Play Store. Once that's confirmed (had to fix a minor mapping issue yesterday, seems stable since), other details like ratings, price etc will follow to replace my "old code" grown since 2014.

Well, TL;DR: Do I have your "Go" to do the polishing here as lined out above? Maybe your last sentence was just ambiguous – and you did in fact mean using the final version from here, once completed, and bring it "over there"?

@BaseMax
Copy link
Owner

BaseMax commented Dec 10, 2020

Yes, you can.

Thanks for your description.
It's good job: mix some different sources.

Maybe your last sentence was just ambiguous – and you did in fact mean using the final version from here, once completed, and bring it "over there"?

GooglePlayWebServiceAPI class used at GooglePlayDatabaseMirror repository.
link: https://github.com/BaseMax/GooglePlayDatabaseMirror/blob/main/src/google-play.php

@IzzySoft
Copy link
Contributor Author

Done. Also increased version in the file's header (was tempted declaring it "1.0", which it probably should be: according to semantic versioning, this is a release "breaking backward compatibility" (e.g. $apps = $parseSearch('foobar') no longer has the result set directly in $apps but instead in $apps['data']).

Please review the PR thoroughly. If you consider it "finalizing", maybe set version to "1.0" – otherwise do not merge but point out what is missing, so that gets addressed first. Thanks!

@BaseMax BaseMax closed this as completed Dec 10, 2020
@BaseMax
Copy link
Owner

BaseMax commented Dec 10, 2020

Thanks,
I create a tag: v1.0.0

We still not have a good example file. Maybe need to adding some different example to example.php file.
Even wider than the wiki

@BaseMax BaseMax reopened this Dec 10, 2020
@BaseMax
Copy link
Owner

BaseMax commented Dec 10, 2020

@IzzySoft
Copy link
Contributor Author

I create a tag: v1.0.0

Cool!

We still not have a good example file.

I just thought to point to the wiki, which already has several examples and basically covers all use cases (and which I need to update again, now that the PR has been merged). I don't think it's necessary to have even more "examples" in some PHP file. Running a series of different calls isn't very helpful, so one needs to copy/paste anyway. I'd rather have 2 or 3 general examples there (for a quick test to see "ah, it's working!") and point to the wiki for further details. Duplicating everything doesn't ease maintenance – just the opposite.

But well, that's my opinion. I won't keep you from creating the "wider variant" 😉

@BaseMax
Copy link
Owner

BaseMax commented Dec 10, 2020

Yes. You prepared the wiki very well.
We can copy examples code from wiki to examples.php file.

@IzzySoft
Copy link
Contributor Author

You prepared the wiki very well.

Thanks!

I've just brought the wiki pages up-to-snuff now. You should take a look at them using Github's new dark theme – be prepared to be blown away (before reading 🙊)

We can copy examples code from wiki to examples.php file.

Be welcome to copy what you think should be copied 😉

Speaking of copy: with the entire framework of my engine using GPLv2, and this class here almost entirely being rewritten by me now: would you be OK with me declaring it GPLv2 in my framework when I make it public one day? There's no ETA for that (it would need a lot of cleanup before from all the embedded test cases and debug code), so currently the question is rather hypothetical. And should it one day happen, I'll of course point here, credits and all, incl. pointing out the different license.

Background: to my knowledge, MIT and GPLv2 have some "compatibility issues". So I just want to make sure not to fall into some trap then.

@BaseMax
Copy link
Owner

BaseMax commented Dec 10, 2020

Be welcome...

Done. cee47f7

This repository is licensed under MIT right now.
Just curiosity, Can I ask your reasons for choosing GPLv2 license? And why not to MIT?

@IzzySoft
Copy link
Contributor Author

Just curiosity, Can I ask your reasons for choosing GPLv2 license? And why not to MIT?

Sure. GPLv2 is a "viral" copy-left license. If someone wants to use the code, the resulting product must use GPLv2 as well and be made public when the product is published – quid pro quo. And other than GPLv3 it does not allow to be monetarized in a service without its stack being again made available openly under GPL (so one can setup the very same service freely). I still consider switching to AGPLv3+ which is similar in that aspect; GPLv3 dropped it.

I have no issues with others using my code (else I wouldn't make it open source). But I don't like the idea of others trapping people into walled gardens with my code, making money with it and not sharing that with me, or at least contributing back in any way. MIT would allow that, as would Apache-2.0. Hence I'd choose either GPLv2 or AGPLv3+ (currently, all my code is under GPLv2 – while all my books, articles and other reading material uses CC BY-NC-SA for the very same reasons).

If you want details, just use your favorite search engine and look for "google hates agpl" 😆 Examples:

Quoting from the former:

Google has made bazillions of dollars using free software in their backend without having to release any of their modifications back into the world. Something like the AGPL which requires server-side modifications to also be released is a direct threat to their way of doing business.

And from the latter:

Because Google's core products are services that users interact with over a remote network interface (Search, Gmail, Maps, YouTube), the consequences of an engineer accidentally depending on AGPL for one of these services are so great that we maintain an aggressively-broad ban on all AGPL software to doubly-ensure that AGPL could never be incorporated in these services in any manner.

Why? It would force them to open-source that stuff 🤣 And I want more open source, hence I like to spread that "virus".

@IzzySoft
Copy link
Contributor Author

@BaseMax is there something missing here, or shall we close this issue?

@BaseMax
Copy link
Owner

BaseMax commented Jun 8, 2023

It seems this issue is too old for 3 years ago :) Nice man.

@IzzySoft
Copy link
Contributor Author

IzzySoft commented Jun 8, 2023

Hehe, then maybe we close it and open a new one should we detect some "missing stuff"? Should I see some while working on something, I'd simply adjust that along the path.

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 a pull request may close this issue.

2 participants