cleanup: protected *and* deprecated methods

[guicamest]

Description of changes:

Cleans up some protected and Deprecated methods. As with #202, it may break clients using this library that expect these methods to be there. Therefore, either they can be contemplated for a 2.0 release, or alternatively with a disclaimer in a 1.X release.

The changes remove 56 javadoc warnings during compilation of the project, as well as increasing coverage a bit:

Before:

After:

Before:

After:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[markjschreiber]

Deleting protected methods would be a breaking change. Perhaps we can create a v2 working branch and make these changes there?

[guicamest]

Sure! Could you create it please 🙏 ? I'll change the target branch of this PR once it is created.

Should I move the change of the examples location to that branch too? It is also a potential breaking change since those public classes won't be in the jar.

[markjschreiber]

Sure! Could you create it please 🙏 ? I'll change the target branch of this PR once it is created.

Should I move the change of the examples location to that branch too? It is also a potential breaking change since those public classes won't be in the jar.

Actually, an alternative would be to change the version number in gradle.build in your PR which would move our main to version 2.x

[guicamest]

Sure I can!

I was thinking of doing some more breaking changes related to the public api and the visibility of the classes. Some examples:

• newDirectoryStream does not throw IOException -> clean up
• public static S3Path getPath(S3FileSystem fs, S3Object s3Object) in S3Path is not used, can be removed.
• public String bucketName() in S3Path doesn't need to be public, the visibility can be reduced.
• S3Path and other classes visibility can be reduced as well, in order to avoid leaking the classes to the user of the library who could potentially write some code with this information. This will benefit the maintenance of the library, as it'll provide more freedom to implementation changes without risking breaking user code. After all, this library is an implementation of NIO so that users shouldn't care if a Path is an S3Path or AWSS3Path. This is how the default implementations are implemented, for example, class LinuxFileSystem is not public.

Additionally, after doing this changes, the revapi library can be added so that whenever a commit is added that brakes API/ABI compatibility, the build fails. There is a blog post on how it works.

What do you think?

[markjschreiber]

Agreed, if we declare that main is now going to be for development of a future 2.0 then we can start bringing these breaking changes in. Ideally all of them before we tag a 2.0 release. If we need to we can make interim release candidates.

[markjschreiber]

If you include a commit in this PR that includes a change in gradle.build to a version like 2.0 or 2.0-dev then I can approve breaking changes. Alternatively you (or I) could make a separate PR that does this and then you could rebase this PR on top of that one. Let me know what your preference would be.

[guicamest]

If you include a commit in this PR that includes a change in gradle.build to a version like 2.0 or 2.0-dev then I can approve breaking changes. Alternatively you (or I) could make a separate PR that does this and then you could rebase this PR on top of that one. Let me know what your preference would be.

I'll add a commit on this PR later today 👍

[markjschreiber]

I noticed that something has happened with the "WalkFromRoot" and "ListPrefix" examples. They no longer produce output. Not sure if this is due to some changes with the restructuring of packages or the S3FileSystemProvider?

[guicamest]

🤔 I'll take a look 👀

Are you running it via the IDE against an S3 account?

[markjschreiber]

Yes, running against a real bucket

[guicamest]

Found the reason, it is related to #202, I forgot to add the project as a dependency of examples 😞 . I'll submit a PR later today 👍

[markjschreiber]

Thanks. It's possible they have been broken a while independently of this change and I didn't notice because they still exited without an error. I was thinking that they should perhaps be changed to make them more "normal" in their use of filesystems rather than explicitly calling "S3" implementation classes.

[guicamest]

I was thinking that they should perhaps be changed to make them more "normal" in their use of filesystems rather than explicitly calling "S3" implementation classes.

Agreed, let's make it an improvement issue 🙏

Regarding the current implementation, there is an issue with newFileSystem, I'll detail it in the description of this PR