Simple interface #59

arnoFleming · 2019-02-21T21:15:26Z

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to wetransfer_ruby_sdk?
Put an x in the boxes that apply

Docs (missing or updated docs)
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement / maintenance (removing technical debt, performance, tooling, etc...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING doc
I have added necessary documentation (if appropriate)
Code changes are covered by unit tests.
Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

- a Client instance can create a tranfer (and add files on the go) - split Transfers' .new and .create, - new only initializes, - create also sends it to WeTransfers' Public API - need to have a seperate add_file method to make this work - MiniIO introduced to get wrap all things IO, in a very slim interface

- - DRY up request headers - NullMiniIO: all methods are empty - rename old spec files so they wont run, move them to a specific directory transform request params to JSON as late (and as once) as possible add more old spec files to the ignore list - Add RemoteFile and RemoteTransfer modules that change the corresponding instances *in place*. It sets instance variables and getter methods. - Transfer.find works, very limited test, though - Make upgrading a transfer (a bit) less magical. finalize call for transfers operational File upload implemented Implement file completion Refactorings for readability, less complexity Move communication to CommunicationHelper class More focus in integration test Expose #to_json on a transfer Refactoring * use class<<self instead of private_class_method on def self.foobar things refactor: Remove duplication remove unused image refactor get rid of useless example

Start documenting the behavior better

…eton Move specs around

small refactorings for chainging methods on a transfer, and unfuddle the :ensure_ok_status!

Document that 0.10.x only offers transfer support Document high level usage better.

julik

Great progress 🚀 I would say the only thing I am wary of is the reuse of file names as identifiers for files when uploading "in steps". The rest is cosmetic.

julik · 2019-03-13T13:33:06Z

README.md

-  upload.add_file(name: 'README.txt', io: StringIO.new("You should read All the Things!"))
+transfer = client.create_transfer_and_upload_files(message: 'All the Things') do |transfer|
+  # Add a file using File.open. If you do it like this, :name and :io params are optional
+  transfer.add_file(io: File.open('Gemfile'))


If we provide auto-DWIM-ing of the filename from the File object we pass in io: it would be good to document how it works exactly. For example, will it only use the basename? Will it call #path on the file? I think I would even go as far as make the method "overloaded" - hit a different code path when you pass in a file: keyword argument and nothing else, and raise if you, say, pass both file: and name:. The rationale being mostly: if we provide "'magic'"™® it should be easy for the end-user to understand.

Well, The SDK relies on File.basename deeper buried inside as a fallback only. You can rename the file by passing in a name.
Will add documentation to express this behaviour.

julik · 2019-03-13T13:35:05Z

README.md

+  # Add a file with File.open, but give it a different name inside the transfer
+  transfer.add_file(
+    name: 'hello_world.rb',
+    io: File.open('path/to/file/with/different_name.rb')


The README in this case encourages leaking file descriptors. For example, if I encountered a README like this I would probably assume that it is the add_file call that is going to perform #read on the passed IO objects, and I can (and should) actually call it like this:

File.open('/path/to/myfile', 'rb') do |fi| transfer.add_file(name: 'myfile', io: fi) end

but with this example this will probably break as the actual reading of the file data will happen at block exit. Can there be a way to demonstrate it in the readme?

Let's have an IRL conversation, since I'm unsure if I understand the ins and outs of your point.

julik · 2019-03-13T13:35:59Z

README.md

+  # Specifying the size is not very useful in this case, but feel free to explicitly
+  # communicate the size of the coming io.
+  #
+  # The :name param is compulsory if it cannot be derived from the IO.


If we say "cannot be derived" we probably need to explain how it will be derived if that is possible (by calling #path?)

Note to self: See here for a closely related issue.

julik · 2019-03-13T13:39:00Z

README.md

+end
+
+# Upload the file. The Ruby SDK will upload the file in chunks.
+transfer.upload_file(name: "small_file", io: StringIO.new("#" * 80))


This enforces addressing files "by name" whereas in practice the server gives you file IDs at transfer create. It is in our best interest to stimulate the consumer of the SDK to either use an abstraction layer that handles ID binding for them:

transfer.files.first.upload(io: ...)

or encourage them to explore the related file metadata:

transfer.files #=> [<RemoteFile id: ...>]

Our approach to "auto-intuiting" the file IDs from the filenames has led to problems as we destructively sanitize the filenames when they enter our systems

The name that the user provided may not be equal to what the WT stack makes of it. This is documented in the Public API. The sdk allows you to call your file 'the-weather-is:.jpg', and reusing that name, while it will be
But from the high-level view of the SDK, I want users to not worry about this. Just as the sdk hides the uploading in chunks, this method hides the file naming details.

Can you agree to this if this is documented better, or do you want the SDK to get rid of this, and use the WT file naming logic only?

julik · 2019-03-13T13:39:51Z

README.md

 ## Boards

+**NOTE**: **Boards are disabled from version 0.10.x** of the Ruby SDK. The latest releases that include **board support is found in version 0.9.x**


If we remove Boards support from this version probably the documentation has to be removed as well

I will remove the docs, but keep the comment about version 0.9, so peeps will know where to go to.

julik · 2019-03-13T13:47:27Z

lib/we_transfer/communicator.rb

+        transfer.instance_variable_set "@#{i_var}", data[i_var]
+      end
+
+      RemoteFile.upgrade(


Still on the fence about the "upgrade" terminology. What does it mean that a transfer has been "upgraded" versus when it hasn't?

Will express intent better. This is cruft I overlooked after fixing the Remotefile .upgrade fubar.

julik · 2019-03-13T13:48:57Z

lib/we_transfer/mini_io.rb

+    # WeTransferFile
+    #
+    def name
+      File.basename(@io)


Can you really do File.basename(<#File>)? I thought it only worked with path strings

Will investigate

julik · 2019-03-13T13:55:27Z

spec/features/client_creates_a_transfer_spec.rb

+
+  let(:client) { WeTransfer::Client.new(api_key: ENV.fetch("WT_API_KEY")) }
+
+  it "Convenience method create_transfer_and_upload_files does it all!" do


Ok this is probably being too annoying but when naming RSpec stuff the following guideline can often be useful:

If we string all the describe/context/it block titles together, does it create a "Noun + verb" sentence that makes sense when printed? For example:

describe "An invigorated Shlobulator" do context "when connected to a stuffed Frobnicator" do it "confabricates correctly" do .... end end end

gives us the following output:

An invigorated Shlobulator when connected to a stuffed Frobnicator confabricates correctly

which would make sense when printed in one line. With a wording like this we would see

transfer integration Convenience method create_transfer_and_upload_files does it all!

which does not form a sentence. It's a small thing but one of the gains from well-named examples/contexts is that they usually look good no matter which Rspec formatter is used ;)

I had big issues naming this spec, so I am on your side. Will reinvestigate.

julik · 2019-03-13T13:55:52Z

spec/spec_helper.rb

@@ -37,3 +25,5 @@ def test_logger
  config.default_formatter = 'doc'
  config.order = :random
 end
+
+RSpec::Mocks.configuration.verify_partial_doubles = true


julik · 2019-03-13T13:56:30Z

spec/we_transfer/communicator_spec.rb

+          .to raise_error(WeTransfer::CommunicationError)
+      end
+
+      it "showing the error from the API to the user" do


Same as above re. example titles

arnoFleming

Thanks for all good tips, will push some changes.

arnoFleming · 2019-03-14T09:30:47Z

lib/we_transfer/communicator.rb

+
+    def request_as
+      @request_as ||= Faraday.new(API_URL_BASE) do |c|
+        minimal_faraday_config(c)


arnoFleming · 2019-03-14T09:31:50Z

lib/we_transfer/communicator.rb

+        transfer.instance_variable_set "@#{i_var}", data[i_var]
+      end
+
+      RemoteFile.upgrade(


Will express intent better. This is cruft I overlooked after fixing the Remotefile .upgrade fubar.

arnoFleming · 2019-03-14T09:32:21Z

lib/we_transfer/mini_io.rb

+    # WeTransferFile
+    #
+    def name
+      File.basename(@io)


Will investigate

arnoFleming · 2019-03-14T09:33:36Z

spec/features/client_creates_a_transfer_spec.rb

+
+  let(:client) { WeTransfer::Client.new(api_key: ENV.fetch("WT_API_KEY")) }
+
+  it "Convenience method create_transfer_and_upload_files does it all!" do


I had big issues naming this spec, so I am on your side. Will reinvestigate.

arnoFleming · 2019-03-14T09:35:33Z

README.md

-  upload.add_file_at(path: '/path/to/another/local/file.jpg')
-  upload.add_file(name: 'README.txt', io: StringIO.new("You should read All the Things!"))
+transfer = client.create_transfer_and_upload_files(message: 'All the Things') do |transfer|
+  # Add a file using File.open. If you do it like this, :name and :io params are optional


Will propose a change, keep you posted

arnoFleming · 2019-03-14T10:01:54Z

README.md

+end
+
+# Upload the file. The Ruby SDK will upload the file in chunks.
+transfer.upload_file(name: "small_file", io: StringIO.new("#" * 80))


The name that the user provided may not be equal to what the WT stack makes of it. This is documented in the Public API. The sdk allows you to call your file 'the-weather-is:.jpg', and reusing that name, while it will be
But from the high-level view of the SDK, I want users to not worry about this. Just as the sdk hides the uploading in chunks, this method hides the file naming details.

Can you agree to this if this is documented better, or do you want the SDK to get rid of this, and use the WT file naming logic only?

arnoFleming · 2019-03-14T10:02:36Z

README.md

 ## Boards

+**NOTE**: **Boards are disabled from version 0.10.x** of the Ruby SDK. The latest releases that include **board support is found in version 0.9.x**


I will remove the docs, but keep the comment about version 0.9, so peeps will know where to go to.

arnoFleming · 2019-03-14T10:03:31Z

lib/we_transfer/client.rb

+      transfer
+        .upload_files
+        .complete_files
+        .finalize


I could even get rid of the transfer helper var, but it expresses intent so beautifully :)

arnoFleming · 2019-03-14T12:28:16Z

lib/we_transfer/communicator.rb

+    UPLOAD_URL_URI = "/v2/transfers/%s/files/%s/upload-url/%s"
+
+    DEFAULT_HEADERS = {
+      "User-Agent" => "WetransferRubySdk/#{WeTransfer::VERSION} Ruby #{RUBY_VERSION}",


Good idea, from both a debugging and spying perspective 🕵️‍♂️.
although the Public API doesn't store it anywhere, and I cannot seem to find it in transfer either.

arnoFleming · 2019-03-14T12:29:18Z

lib/we_transfer/mini_io.rb

+# frozen_string_literal: true
+
+module WeTransfer
+  # Wrapper around an IO object, delegating only methods we need for creating


reword: creating files and sending in chunks

arnoFleming force-pushed the simpler-interface branch from c5cd01f to dfd22cf Compare February 22, 2019 09:43

arnoFleming changed the title ~~--wip-- [skip ci]~~ Simple interface Feb 22, 2019

arnoFleming force-pushed the simpler-interface branch from fd8f170 to 4a9b6d3 Compare March 1, 2019 10:35

arnoFleming requested review from julik, davidenko87, arkaitzgarro and a team and removed request for julik and davidenko87 March 1, 2019 15:58

arnoFleming force-pushed the simpler-interface branch from 89cf005 to 4fab8b0 Compare March 1, 2019 16:01

arnoFleming marked this pull request as ready for review March 1, 2019 16:02

arnoFleming added 2 commits March 2, 2019 14:50

arnoFleming force-pushed the simpler-interface branch from 4fab8b0 to 9ee34dc Compare March 2, 2019 13:51

arnoFleming added 2 commits March 2, 2019 15:27

Rename communication module

8138841

Start documenting the behavior better

Enable more jrubies in CI

bcf0a45

arnoFleming force-pushed the simpler-interface branch from 83cc068 to bcf0a45 Compare March 2, 2019 15:10

Let's not try to be ready for frozen strings

963ea55

arnoFleming force-pushed the simpler-interface branch from dde3876 to 963ea55 Compare March 2, 2019 20:16

arnoFleming added 4 commits March 2, 2019 21:25

Implement create_transfer_and_upload_files

e4b1e7b

Use Forwardable over silly method defs that do just that

73bfb10

Spec files', multiparts' #to_h and transfers' #to_h and #to_json

f272590

Make Communication class safer by not storing state inside some singl…

6d1f239

…eton Move specs around

arnoFleming force-pushed the simpler-interface branch from 5242b54 to 6d1f239 Compare March 6, 2019 21:09

arnoFleming added 6 commits March 7, 2019 17:08

Rename to Communicator,

d578838

small refactorings for chainging methods on a transfer, and unfuddle the :ensure_ok_status!

Bump version to 0.10.x

af4eaae

Document that 0.10.x only offers transfer support Document high level usage better.

freeze those strings

8d0f98c

Adds ability to find files by their id as well

30f68f9

Find file simplified and documented

ac9a7e0

Adds more or better documentation

22c3bd8

julik reviewed Mar 13, 2019

View reviewed changes

arnoFleming commented Mar 14, 2019

View reviewed changes

arnoFleming added 3 commits March 22, 2019 16:18

More Yardoc

789e3b3

Add version to the required files

97b1443

Raise with more expressiveness

5541469

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple interface #59

Simple interface #59

arnoFleming commented Feb 21, 2019 •

edited

Loading

julik left a comment

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

arnoFleming Mar 14, 2019

julik Mar 13, 2019

julik Mar 13, 2019

arnoFleming left a comment

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

arnoFleming Mar 14, 2019

		## Boards

		NOTE: Boards are disabled from version 0.10.x of the Ruby SDK. The latest releases that include board support is found in version 0.9.x


		let(:client) { WeTransfer::Client.new(api_key: ENV.fetch("WT_API_KEY")) }

		it "Convenience method create_transfer_and_upload_files does it all!" do

Simple interface #59

Are you sure you want to change the base?

Simple interface #59

Conversation

arnoFleming commented Feb 21, 2019 • edited Loading

Proposed changes

Types of changes

Checklist

Further comments

julik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnoFleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnoFleming commented Feb 21, 2019 •

edited

Loading