Append filename to destination if source is file

[genziano]

Fixes #74.

The CloudPath.download_to method adds the filename to the destination path if the cloud source is a file and the destination is a folder.

[jayqi]

The change itself looks good! For the tests, I would recommend adding a new test instead of adding to test_file_read_writes. test_file_read_writes is a bit big and unwieldy, and having a more targeted unit test is probably better for maintainability anyways.

[jayqi]

By the way, if you write something like "Fixes #74" in your PR description, GitHub has special functionality to link this PR to that issue and will close the issue automatically when the PR is merged. If it's working, you'd see that the keyword has a dotted underline. The mechanics and triggering keywords are documented here:

https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[genziano]

By the way, if you write something like "Fixes #74" in your PR description, GitHub has special functionality to link this PR to that issue and will close the issue automatically when the PR is merged. If it's working, you'd see that the keyword has a dotted underline. The mechanics and triggering keywords are documented here:

https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Nice one!

[genziano]

The change itself looks good! For the tests, I would recommend adding a new test instead of adding to test_file_read_writes. test_file_read_writes is a bit big and unwieldy, and having a more targeted unit test is probably better for maintainability anyways.

Good idea, I will separate that test. What do you think about having a class for testing, say TestCloudPath, where its methods implement tests for CloudPath methods?

[jayqi]

What do you think about having a class for testing, say TestCloudPath, where its methods implement tests for CloudPath methods?

For this project, we're using test modules (e.g., test_cloudpath_file_io) and descriptive test names for organizing our tests. From that regard, I don't think it's necessary to add test classes as another layer of organization. For parameterizing tests for the different types of cloud paths and handling setup/teardown, we're using fixtures—specifically the rig fixture. You can see how the rigs are set up on conftest.py. So, unless you have a compelling idea for how a test class could further simplify our testing code, I'm not in favor of using test classes.

[genziano]

What do you think about having a class for testing, say TestCloudPath, where its methods implement tests for CloudPath methods?

For this project, we're using test modules (e.g., test_cloudpath_file_io) and descriptive test names for organizing our tests. From that regard, I don't think it's necessary to add test classes as another layer of organization. For parameterizing tests for the different types of cloud paths and handling setup/teardown, we're using fixtures—specifically the rig fixture. You can see how the rigs are set up on conftest.py. So, unless you have a compelling idea for how a test class could further simplify our testing code, I'm not in favor of using test classes.

I never used test classes myself, but I was curious to hear if you thought it could be a good idea.

I see that most live tests are failing, does it have anything to do with the fact that this is a PR from a forked repository, for which there are no credentials?

[jayqi]

A few requested revisions.

I see that most live tests are failing, does it have anything to do with the fact that this is a PR from a forked repository, for which there are no credentials?

Yes, that's the case. Sorry for the inconvenience—I'll think about how we can make this process better. What we did for a previous PR was for one of us (the maintainers) to copy the fork's branch over to the main repository so that the CI can run with the necessary credentials. For now, let's continue development in this PR until we're happy with the changes (using the CI steps before the live tests as a quality check). When it's ready, we can do the same approach of copying the branch to the main repo.