Update Objects.js

[Relik77]

Fix content encoding in "Get object content" function. Return binary data as a buffer instead of data encoded in utf8 (because data encoded in utf8 is corrupted for binary files).

@see https://www.npmjs.com/package/request#requestoptions-callback

encoding - encoding to be used on setEncoding of response data. If null, the body is returned as a Buffer. Anything else (including the default value of undefined) will be passed as the encoding parameter to toString() (meaning this is effectively utf8 by default). (Note: if you expect binary data, you should set encoding: null.)

[Mattia-Longhin]

Hi @Relik77,

Thank you for your help! Unfortunately there is a problem with this PR: first commit was OK, the second one broke the library. 🙂

Basically, what you did is that you created two nested promises. The external one in save method and the internal one in saveFile method. Your code calls the resolve of the internal promise but never calls the resolve of the external one. You have the desired effect anyway, which is storing the file, but since the promise of save method never get resolved you cannot chain promises with then or async await otherwise your program hangs indefinitely.

I you really want to do that, you should have written something like:

    if (_.isString(fileContent) && fs.existsSync(fileContent)) {
        resolve(await this.saveFile(fileContent, path));
    }

but, in my opinion, this is not the very nice. I think that is preferable to have two separate methods that accepts two different types of arguments instead of having one method that accepts all.

By the way:

• I think that you should not mix different subject on the same PR. The first commit was about an issue with encoding, the second commit is something completely different, it's a new feature. These two thinks belongs to two different PRs.
• I think that you should use more descriptive commit names. Both commits and the PR have the same name, Update Object.js, which is quite unclear.

This PR is not acceptable anymore. I think that this should be rejected and you should create two separate PR: the first one that solves #10, and the second one that proposes the new functionality.

Just ask the maintainer 🙂: @dimer47, what do you think about this?

Have a nice day!

[Relik77]

Hi, I agree.

I initially wanted to do 2 PR but the interface merged the 1st with the 2nd: /

I will correct the code.

The initial idea of making a call to "saveFile" from "save" was just to not break the behavior of the original method, while trying to have a more meaningful name to distinguish the 2 methods:

• the one which takes the path of a file as input;
• and the one which takes its content directly and therefore does not require having the file saved locally on the server;

[dimer47]

Hi @Relik77,

I agree with @Mattia-Longhin this is a new feature. You have a great idea to implement this method ! Thank you again for you contribution. But it's possible to re-submite a new PR excluding encoding change on get() method ?

For this other subject, I propose to you an alternative on issue, I wait your opinion. On open a new PR excluding encoding, I accept you PR to merge changes.