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

Add advanced usage sections to README.rst #741

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Nov 22, 2022

Title

Add advanced usage sections to README.rst (ACL, StorageClass, Metadata etc)

Motivation

worth updating the smart_open docs somehow?

Because if it tripped you up, it might trip up others too. So I wonder if there's a better way to record / present that knowledge.

Originally posted by @piskvorky in #738 (comment)

Tests

Docs only

Work in progress

--

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@mpenkov
Copy link
Collaborator

mpenkov commented Nov 22, 2022

Maybe put these in https://github.com/RaRe-Technologies/smart_open/blob/develop/howto.md instead?

Stuff like this feels a bit too specific for the general readme (the readme links to the howto anyway).

@ddelange
Copy link
Contributor Author

What to do about the doctests there? No easy way to mock s3, azure, and gcs writes I guess?

@mpenkov
Copy link
Collaborator

mpenkov commented Nov 23, 2022

For S3 we can use our own bucket (maybe later...).

For Azure and GCS there isn't much we can do, as a team we don't use those cloud providers.

@ddelange
Copy link
Contributor Author

ddelange commented Nov 23, 2022

we don't use those cloud providers

Same goes for me...

Can moto be enabled for doctests by running them via pytest? That would cover the s3 part at least.

To test azure and gcs, in pypicloud we spin up (fake) servers, configure the client objects accordingly which are then propagated to the final smart_open calls. Is it an option for smart_open to spin those up before running unit tests and doctests? Side-effect would be you can drop (maintenance of) the fake classes for the tests, and run the functional assertions against the fake servers instead.

@ddelange ddelange marked this pull request as draft November 23, 2022 08:07
@mpenkov
Copy link
Collaborator

mpenkov commented Nov 24, 2022

I don't see an optimal solution here.

  1. Do nothing (the status quo): we risk parts of the documentation going stale or suggesting something that doesn't actually work, which is bad
  2. We invest time and effort, and test under moto (and whatever the equivalent is for Azure and GCS). Not sure if this is possible, though. Plus, makes the documentation harder to read.
  3. As above, but with localstack. Should be possible by e.g. sneaking the endpoint URL via environment variables. Makes the documentation harder to read.
  4. We invest time, effort and money, and sign up for each supported backend (AWS, Azure, GCS, and whatever gets added next).

@ddelange
Copy link
Contributor Author

Thanks for the reply! And fair enough, let's go with the status quo :shipit:

@ddelange ddelange closed this Nov 24, 2022
@piskvorky
Copy link
Owner

I don't think the ticket was related to Azure or GCS, was it?

Any way to help the original user / fix the discovery issue?

@ddelange
Copy link
Contributor Author

for S3 at least, howto covers it I think: it explains the mechanic and suggests to study the boto3 api reference

@ddelange
Copy link
Contributor Author

ddelange commented May 15, 2023

Hey @piskvorky @mpenkov 👋

These last months, I've had to point quite a few people to this PR for reference.

Can you reconsider this PR against README.rst?

  • It is very brief (+35 LOC only)
  • It showcases the slightly different transport_params semantics for the big three
  • It is a unified structure, basically a 'comparison'
  • Not subject to the doctest blocker mentioned above

I think this addition will save many devs a journey!

@ddelange ddelange reopened this May 15, 2023
@ddelange ddelange marked this pull request as ready for review May 15, 2023 05:46
@ddelange
Copy link
Contributor Author

ddelange commented Jun 9, 2023

Is <provider> Advanced Usage maybe a misleading subheader for README?

Does something like <provider> Custom Uploads make more sense?

@mpenkov mpenkov self-assigned this Sep 7, 2023
@mpenkov mpenkov added this to the Next release milestone Sep 7, 2023
@mpenkov mpenkov merged commit d4b82ce into piskvorky:develop Feb 22, 2024
42 checks passed
@mpenkov
Copy link
Collaborator

mpenkov commented Feb 22, 2024

Merged, thank you

ddelange added a commit to ddelange/smart_open that referenced this pull request Feb 22, 2024
…open into patch-2

* 'develop' of https://github.com/RaRe-Technologies/smart_open:
  fix test, for real this time
  update integration test
  Add advanced usage sections to README.rst (piskvorky#741)
  Add logic for handling large files in MultipartWriter uploads to s3 (piskvorky#796)
  Fix __str__ method in SinglepartWriter (piskvorky#791)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants