Refactor Work Files API to not shadow built-in `open`

[BigRoy]

This resolves #407 so that the Work Files API does not shadow the built-in function open and refactors it to open_file. To keep a consistent API I've implemented the same change for save to save_file.

What's changed?

Update the Work Files API:

• Refactor open() -> open_file()
• Refactor save() -> save_file() to match with open_file.

This is updated for the implemented hosts, the Work Files tool README that describes the API and the work files tool itself of course.

[tokejepsen]

I guess we dont want to and cant, maintain backwards compatibility?

[mottosso]

I don't get it. What problem does this solve?

The original issue mentions..

That's bad practice and if you're not careful it may lead to issues.

What issues specifically?

The "bad practice" part must be in reference to declaring open in the same scope, without a namespace.

open = "c:\my_file.txt"
with open(open) as f:
  f.read()
# Having a bad time

But surely, with a namespace this could not possibly be an issue?

workfiles.open("c:\my_file.txt")
# Having a good time

It's not an ideal name, but this is an API we're changing. APIs cannot break; like diamonds they are forever. Either keep both open and open_file around, or leave open as-is.

[tokejepsen]

Might need to tag in the issue author @jasperges

[BigRoy]

Isn't it bad practice to shadow built-ins anywhere as one might accidentally get bitten by it later? I think it's general bad practice.

Anyway, I've solely set up this PR to correspond with the related issue and because I'm personally fine with it. Also, because it aligns the methods more with the others like current_file() which is not current() plus it resolved the shadowing, as such I agree style wise it's a better choice.

Regarding it being an API. Sure. I understand the backwards compatibility notes. However, I am not sure if this ever was part of any front-facing API that was to be used outside of Avalon. It was only the design implemented for the work files tool. But looking at @tokejepsen his comment it seems the function have been referenced outside that scope too. (Or it was just a general question.)

Feel free to do with it what fits best. :) Maybe @jasperge has other notes to add.

[tokejepsen]

Regarding it being an API. Sure. I understand the backwards compatibility notes. However, I am not sure if this ever was part of any front-facing API that was to be used outside of Avalon. It was only the design implemented for the work files tool. But looking at @tokejepsen his comment it seems the function have been referenced outside that scope too. (Or it was just a general question.)

Ahh yeah, its not in the API so should be alright to rename. I'm guessing...

[davidlatwe]

The "bad practice" part must be in reference to declaring open in the same scope, without a namespace.

Yeah, that's correct. But I think it would be a real (internal) bad practice if we need to use the built-in open inside workio.py in the future.

So I think we do need this be fixed, but also need to expose the open in each host api for backward compat. :)

Edit: Although it's not in the scope of the top level API, but it's on top of each host, so I think the backwards compatibility still need to be take cared.

[BigRoy]

Edit: Although it's not in the scope of the top level API, but it's on top of each host, so I think the backwards compatibility still need to be take cared.

I thought of another issue why this PR is necessary. The open function is also exposed in __init__.py for all the hosts. That could be dangerous too, right? It's not solely exposed in the workio.py

[davidlatwe]

The open function is also exposed in __init__.py for all the hosts. That could be dangerous too, right?

I think it's actually safe, as long as you keep the namespace when you referencing, and the open won't affect other function which used built-in open inside other module.

I did a quick test that has this structure:

./testopen
    /__init__.py  # Importing both following functions in the same scope
    /builtin.py  # Has a function that used the built-in `open`
    /override.py  # Has a function called `open`

And they all worked safely. Unless I am testing in wrong direction ?

[BigRoy]

I think it's actually safe, as long as you keep the namespace when you referencing, and the open won't affect other function which used built-in open inside other module.

That test does seem to cover the use-cases yes. It just means that no __init__.py for any of the hosts could ever use the built-in open, but I guess at this stage it's not a big hurdle.

How about introducing the backwards compatibility only at the end of the __init__.py file?

# Backwards API compatibility
open = open_file
save = save_file

I guess if anyone ever used it from "the host" then they wouldn't have accessed workio.py directly anyway, which also goes against the rules of the API.

@davidlatwe you should be able to push changes directly into this branch for the PR if you want to implement backwards compatibility. Or let me know if you want me to add those lines and push it to the branch.

[mottosso]

It just means that no init.py for any of the hosts could ever use the built-in open

It's not that serious; you can always __builtins__.open() if you need it. open in the global scope is convenience at best, and sometimes it makes sense to override it. Like type.

[davidlatwe]

How about introducing the backwards compatibility only at the end of the __init__.py file?

That's what I have in mind, yes :D And sure, I'll push them later :muscle:

[davidlatwe]

Oh no, I could not push into this branch, need a hand @BigRoy :cry: (I guess the allow edit from maintainer option was off ?)

Edit: I also spotted that the line here and here should also be changed, too. Well be there once I able to push the commits :)

[BigRoy]

If checked, users with write access to getavalon/core can add new commits to your fix407 branch. You can always change this setting later.

That option is checked. Hmm...

Anyway, on it!

[BigRoy]

@davidlatwe let me know if it's all good like this. 👍

[davidlatwe]

Merge !