Deprecate Store.get_s3fs_session, expose under new name: Store.get_s3_filesystem

[chuckwondo]

The method Store.get_s3fs_session is misleading because it does not return any kind of "session" object. Rather, it returns an instance of s3fs.S3FileSystem, so the method should be renamed to properly reflect the type of object it returns.

I propose we deprecate Store.get_s3fs_session (along with all other places where it is surfaced through functions of the same name) and replace it (and other methods/functions named the same) with the name get_s3fs_filesystem.

[mfisher87]

Or get_s3_filesystem since fs already stands for file system?

[chuckwondo]

Or get_s3_filesystem since fs already stands for file system?

Yep, I was actually going to suggest that originally for that same reason, so I'm happy to use that name instead.

[Sherwin-14]

@mfisher87 Hey I want to have a look into this one.

[chuckwondo]

@Sherwin-14, I'm going to pick up this issue. I've been dealing with this very problem in other work that I'm doing, and the solution is not obvious, nor even documented.

Sorry, I was thinking about #765

[mfisher87]

Remember for the purpose of the changelog that this is a breaking change :)

[chuckwondo]

Remember for the purpose of the changelog that this is a breaking change :)

If we deprecate the existing methods/functions, it's not a breaking change until we actually remove the deprecated items, no?

[mfisher87]

Correct, if we go down that path we should leave deprecation warnings! I did not pay close enough attention to your use of "deprecate" in the issue description and was just reading the title :)

[chuckwondo]

Correct, if we go down that path we should leave deprecation warnings! I did not pay close enough attention to your use of "deprecate" in the issue description and was just reading the title :)

Right, thanks for clarifying that in the issue title. This raises another question. Have we defined how we deprecate things? In terms of how deprecations appear in the changelog, our changelog conventions cover that, but how about a coding guideline for actually implementing a deprecation?

[mfisher87]

Yeah, that sounds like something that should be in the contributing guide! Great call. Beyond e.g. "use the built-in DeprecationWarning exception type", what things would you imagine should be there? A style guide for the warning message itself? e.g.: "always mention the new canonical function name in the warning", "always link to a migration guide for this specific deprecation"? Coding style for the deprecated function stub, e.g.: "don't maintain code in both places unless the behavior is actually changing; in name change instances, raise the warning then immediately call the new canonical function"?

[chuckwondo]

See thread in PR #768

[chuckwondo]

Fixed by #768