Wraps transaction into separate class

[mitchhentges]

This breaks apart EditService:

• Transactions (including committing) are now represented by:
  with WritableGooglePlay.transaction() as google_play:
• Managing real vs mock API has been split into the GooglePlayConnection abstraction
• Read-only logic is simpler than write logic (no need to commit after), so it's split into a different class and made easier to use.

---

@JohanLorenzo I'm pretty happy with these changes, though I'm on the fence about splitting ReadOnlyGooglePlay from WritableGooglePlay. My main reasoning for doing happened when I saw this:

with GooglePlay.transaction(connection, 'org.mozilla.firefox', do_not_commit=True) as google_play:
    check_rollout(google_play, config.days)

When you're doing work that just requires read-only access, it's noisy to add do_not_commit=True, and setting up the whole with statement is annoying. I'm especially worried that it will be too easy to omit the do_not_commit parameter, causing read-only work to take longer, since an unnecessary commit happens.
So, by splitting the read-only class from the writable one, we get:

google_play = ReadOnlyGooglePlay.create(connection, 'org.mozilla.firefox')
check_rollout(google_play, config.days)

Devs will realize the existence of ReadOnlyGooglePlay (if there's a WriteableGooglePlay, there's probably a "read"-related one), and will opt to use it if possible, since its less verbose to write. I think this is way better than having to scan for missing do_not_commit parameters or doing unnecessary commits. What do you think?

[coveralls]

Coverage decreased (-0.1%) to 94.697% when pulling b06494c on mitchhentges:209-transaction-type into f41ad26 on mozilla-releng:master.

[JohanLorenzo]

Thanks for taking care of this refactor! I enjoy the split in ReadOnly and Writable and I think we should keep it.
Before merging this PR, I think we should make the context more pythonic. To me, we can define a single entry point à la open() or ZipFile().

I'm thinking of something like this:

# Check rollout 
with GooglePlay(package_name, service_account, credentials_file) as google_play:
    for (release, age) in check_rollout(google_play, config.days):
        # ... 


# Push apk
with GooglePlay(
    package_name, service_account, credentials_file,
    mode='w', commit=commit, contact_google_play=contact_google_play
) as google_play:
    for path, metadata in apks_metadata_per_paths.items():
        # ...


@contextmanager
def GooglePlay(package_name, service_account=None, credentials_file=None, mode='r', commit=False, contact_google_play=True):
    if contact_google_play:
        if not service_account or not credentials_file:
            raise ValueError()
        edit_service = _connect(service_account, credentials_file)
    else:
        edit_service = _MockService()

    if mode == 'r':
        google_play = _ReadOnlyGooglePlay(edit_service, package_name)
    elif mode == 'w':
        google_play = _WritableGooglePlay(edit_service, package_name)
    else:
        raise ValueError()

    yield google_play
    
    if commit:
        edit_service.commit()
    else:
        logger.warning()

This way, consumers of the GooglePlay context don't have to deal with multiple calls to get the right context initialized. What do you think?

I'm not too sure what's the end result is going to look like. That's why I don't r+ yet, I'd like to see it

[JohanLorenzo]

Nit: Let's avoid negative boolean names. It adds a mental layer of indirection when reading the code.

[mitchhentges]

Hey, thanks for the thoughtful response :)

A couple thoughts:

1. IMHO, I'd rather make two calls than pass unnecessary parameters to a function. For example:

with WritableGooglePlay(MockGooglePlayConnection(), package_name) as google_play:
// vs
with WritableGooglePlay(package_name, None, None, contact_google_play=False) as google_play:

This is for a couple reasons:

• Having mutually exclusive parameters adds more noise to the calling interface. It forces readers to consider whether the None values are meant to represent anything, or if they're just filler values
• It's more explicit to provide that separate function call. Theoretically, the contact_google_play flag represents the same information as the second function name (MockGooglePlayConnection()). And, since the relevant parameters are grouped up front, it's more clear what's used for what, without having to grub through docs.

WritableGooglePlay(package_name, GooglePlayConnection('[email protected]', 'file.p12'))
                                                                  |     |
                    these two parameters are related, and it's    |    /
                    clear because they're called in a function    -----    
                    together

WritableGooglePlay(package_name, '[email protected]', 'file.p12', True)
                                             |          |    |
                                             |          /    /
            These parameters are all related ----------------
            but it's not obvious without consulting docs

2. I'm not sure that the mode='w/r' style makes sense for this API. The biggest up-front cost of this decision would be that it isn't obvious what kind of object is returned by the single GooglePlay function without looking at the docs/source code. In the case of ZipFile, the mode style may have made sense since it's lowkey mirroring a file system api (which takes that parameter), as well as since there's a single ZipFile class (unlike the multiple for Google Play).
   Additionally, since there's a one-to-one mapping between mode and class name, I'm unsure what value this is providing

mode='r' # => ReadOnlyGooglePlay(...)
mode='w' # => WritableGooglePlay(...)

One other note is that, if at some point in the future, the two implementations require a different set of parameters (perhaps WritableGooglePlay would need an extra credential, since it performs more powerful operations? Bad example, but w/e 😄), then it would make the single function more leaky, since there'd be an optional parameter that's only used depending on the value in mode.

3. That's enough concerns outta me though, I really like your point that the static .transaction(...)/.create(...) functions not being pythonic, that's a good observation.
   I'll make WritableGooglePlay a context manager class, so you can just:

with WritableGooglePlay(...) as google_play:

and I'll add a commit(...) function.

This way, it's more like ZipFile and others where you have the option of using it as a context manager, but if you want to manually open and close it, you can:

google_play = WritableGooglePlay(...) 
# ...
google_play.commit()

[mitchhentges]

🤔 Looking at this a little further, I'm not sure it makes sense to remove the static function for each of the GooglePlay implementations. Let's focus on ReadOnlyGooglePlay here:
Since there's no clean up possible for the read-only connection, it doesn't make sense to use with a context manager.

However, there is a set-up sequence, where a connection is used to establish a transaction. Importantly, this operation is a "side-effect"-y operation, so it doesn't make sense to do in the constructor.
The cleanest of of managing this is by having a function that takes required parameters, performs the side-effect-y operation, then creates the object and returns it:

def create(connection, package_name):
    edit_resource = connection.spool_it_up(package_name)
    edit_id = edit_resource.activate_turbo()
    return ReadOnlyGooglePlay(edit_resource, edit_id, package_name)

And, once we have this function that is closely related to ReadOnlyGooglePlay, we either name it to show that they're related (create_read_only_google_play(conn, package_name)) or we namespace it (ReadOnlyGooglePlay.create(conn, package_name)).

🤔 That's the costs and benefits affecting my design decisions so far, what do you think?

[JohanLorenzo]

Note: @mitchhentges and I have had a lengthy conversation since last comment, about classes vs raw types, which led to mozilla-releng/releng-rfcs#29.

I've tried to review this PR by not being biased about my general position on that matter, and by just reflecting upon calls that occur in this changeset.

I'm okay to not have the Read/Write split anymore. If the needs comes up, we can bring it back later.

I think some changes are required before landing the PR. I'd like to get another view them once they're done.

[MihaiTabara]

Dropping some initial thoughts on this.

To be honest, I like both options, for different reasons. But overall, I'm slightly leaning towards the pythonic way of doing it, rather than classes.

Reasons I like the using classes approach:

1. solves the extra parameters noise which can be quite annoying to follow in the other case of overloading the function with multiple functionalities
2. no-one likes to toggle between using an object and the docs, ideally things should be self-documenting; objects are easier to read, reducing the time to go back and forth and double-check if a certain piece of logic from the docs validates
3. fits better in the future space of a potential, more mainstream, Python with static-typing hinting (3.7+ onwards)

Reasons I like the pythonic way:

1. this is the pythonic way; most of the widely used modules, like requests or flask are written in that manner
2. in a more generic perspective; altough types are getting more and more traction, we're not yet using them in a wide perspective in our codebase. This is, by no means, an "but this is how we've always done it" scenario, but there may be some underlying reasoning for that. I guess while the hint typing is something that was already introduced, Python will always remain a dynamically typed language so I believe we should stick to the primitives as much as we can. Since static typing is still under development in Python, it is subject to change in its future releases.
3. slightly easier to test/mock. classes add import overhead

With all that said, I still believe consistency is good across an organization so I'm leaning towards the pythonic way of overloading multiple behaviors within the primitive functions.

[mitchhentges]

Thanks Mihai for weighing in on the discussion. Though I'm not super hyped on the result, I'm glad that we can unblock this PR and move forward again 😄

---

this is the pythonic way; most of the widely used modules, like requests or flask are written in that manner

Classes aren't less pythonic 🤔
You mentioned two examples of dictionary use within the python ecosystem: requests and flask. In those specific cases, they use dictionaries for raw HTTP data because it has to be dynamic (data at the raw HTTP layer has to be unstructured by definition 😄)
Here in my RFC, I mean to address this specific case, and how I agree that dynamic data should be held in a dynamic container.

However, in an API boundary with a known set of possible data, it's now possible to represent your data in an explicit structure.

Python will always remain a dynamically typed language so I believe we should stick to the primitives as much as we can.

Yes, it's dynamically typed, but Python is giving us the built-in, native ability to structure known types of data. An argument analogous to this is "computers will always remain operating on primitive processor instructions, so we should program at that primitive level accordingly."

Since static typing is still under development in Python, it is subject to change in its future releases.

Using classes to encapsulate logic and config has been possible since before the release of the first Python 3. They aren't experimental at this point.

classes add import overhead

The amount of weight that this point appears to have has surprised me.

---

Mihai and Johan, you guys are pretty aligned on the direction here, so I'll 🤷‍♂️. I've updated the PR to use raw primitives and removed the GooglePlayConnection abstraction, and removed the two follow-up "abstraction" PRs.

I'm not confident in the direction that we'll take, but I'm really happy to be able to have a clear direction for our team that I can implement this PR against so we can have it landed 😃

[JohanLorenzo]

Thank you @mitchhentges for accepting to implement Mihai's and my preferred way. I appreciate this gesture.

The code looks good to me. I found one small nit. Let's address it before landing. No need for me to have another look 🙂