Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip and quota #393

Merged
merged 3 commits into from
Jan 28, 2021
Merged

Skip and quota #393

merged 3 commits into from
Jan 28, 2021

Conversation

aaschaer
Copy link
Contributor

Adds named arguments for skip_source_errors and fail_on_quota_errors to TransferData and methods for the new GET /task/<task_id>/skipped_errors and GET /endpoint_manager/task/<task_id>/skipped_errors to the TransferClient

Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logical lines of code are definitely fine/correct (although I want to see and read the new API docs before approving, just to triple-check everything).

I have a couple of bits of feedback on inline docs.

@@ -134,6 +134,8 @@ def __init__(
preserve_timestamp=False,
encrypt_data=False,
deadline=None,
skip_source_errors=False,
fail_on_quota_errors=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add these to the list of params above, so that we get doc for these parameters. I think it's okay to just link to T-API docs which explain what they do, if that's convenient. (But obviously that may have to wait for said doc to be finished.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah missed those, added!

"TransferClient.endpoint_manager_task_"
"skipped_errors({}, ...)".format(task_id)
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really matter, but may I suggest

        self.logger.info(
            "TransferClient."
            "endpoint_manager_task_skipped_errors({}, ...)".format(task_id)
        )

as an alternative? The long-line rule may demand that we break things up, but I think the above is more readable because it avoids breaking a "long word".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - I think this might have been auto-formatted since I tend to err in the direction of too long a line ^^"

transfer, only files that were actually transferred, and does not include
any directories.
transfer or skipped due to skip_source_errors, only files that were
actually transferred, and does not include any directories.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is now very long; I think we should break it up.

I suggest:

Only files that were actually transferred are included.
This does not include files that were checked but skipped as part of a sync
transfer, files which were skipped due to skip_source_errors, or
directory names.

But if you have an alternate phrasing you prefer, go for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I slightly adjusted the phrasing to put directories first since that seems the most common case.

@sirosen sirosen merged commit 9bef560 into main Jan 28, 2021
@sirosen sirosen deleted the skip_and_quota branch January 28, 2021 22:06
@sirosen
Copy link
Member

sirosen commented Jan 28, 2021

I made a very minor fix to :param ... in the TransferData docstring, but other than that, all was spot on.
Thanks for handling the fixes, and 👍 to your rephrase of the successfully transferred files explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants