irods / irods_client_globus_connector

The iRODS Globus Connector
2 stars 4 forks source link

[66] Implemented rename (GLOBUS_GFS_CMD_RNTO) (main) #87

Closed JustinKyleJames closed 5 months ago

JustinKyleJames commented 5 months ago

Also produced an error message for commands that are not implemented.

I noticed the checksum case statement was not indented correctly. I updated the curly brackets and break statement to be four spaces from the case but did not update the code between the curly brackets which needs an extra space as I didn't want to confuse the reviewers.

JustinKyleJames commented 5 months ago

Even though it says collection… this renames to a new data object name as well? We doing tests in this repo?

It is a misnomer but that gets set up earlier and can be either a collection or a data object. I don't like that it is that way but went with how it already is.

I can change that if desired. What would be the preferred name if it can be either depending on context?

Seems pretty straight-forward.

Does it work as intended?

Yes, I tested it in both good conditions and where the source object does not exist.

trel commented 5 months ago

I can change that if desired. What would be the preferred name if it can be either depending on context?

Perhaps something like ... target_path or target_logical_path?

JustinKyleJames commented 5 months ago

I can change that if desired. What would be the preferred name if it can be either depending on context?

Perhaps something like ... target_path or target_logical_path?

I changed it to target_path. I'll need to do a bit more testing since this touches all of the commands.

trel commented 5 months ago

That can definitely be a separate PR if you want. Keeps this one focused.

JustinKyleJames commented 5 months ago

That can definitely be a separate PR if you want. Keeps this one focused.

I think that is better. I'll open an issue on it.

JustinKyleJames commented 5 months ago

I added rename collection functionality in the last commit. This has been tested and works.

JustinKyleJames commented 5 months ago

I added a commit to handle one outstanding issue with issue 84. The code now uses transfer_info->alloc_size when making a decision about the number of threads on uploads.

This has been tested and behaves as expected for the following cases:

  1. The alloc_size is not provided.
  2. The alloc_size is greater than or equal to the threshold.
  3. The alloc_size is less than the threshold.
trel commented 5 months ago

if you're happy - please squash em.

trel commented 5 months ago

looks like it will be down to 2 commits... 84 and 66.

JustinKyleJames commented 5 months ago

if you're happy - please squash em.

Done

JustinKyleJames commented 5 months ago

I handled the latest comments and ran it through testing.

Note that I couldn't change the routine to return std::string& due to the return of a local variable when lookup fails.

korydraughn commented 5 months ago

Seems ready to me. If you're happy with it and it passes all the tests, squash it.

JustinKyleJames commented 5 months ago

Seems ready to me. If you're happy with it and it passes all the tests, squash it.

Done

JustinKyleJames commented 5 months ago

Pound it.

Done