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

Fix File Create Docs #429

Merged
merged 6 commits into from
May 9, 2022
Merged

Conversation

gewenyu99
Copy link

@gewenyu99 gewenyu99 commented Apr 20, 2022

Currently, the documentation for creating files passes in a file binary. Our current implementation only accepts a file path. This must be corrected in the documentation for all SDKs.

Node SDK ✅
Deno SDK ✅
PHP SDK ✅
Python SDK ✅
Ruby SDK ✅
Dart SDK ✅ (this hasn't changed)
Kotlin (Java) ✅ (this hasn't changed)
Kotlin (Kotlin) ✅ (this hasn't changed)
Swift ✅ (this hasn't changed)

Related to
appwrite/sdk-for-node#32

@gewenyu99 gewenyu99 changed the title Fix File Create Docs [WIP] Fix File Create Docs Apr 20, 2022
@gewenyu99
Copy link
Author

@lohanidamodar I feel like the behavior of the Dart SDK isn't consistent with the other SDKs. It still takes an object like this:

  Future result = storage.createFile(
    bucketId: 'default',
    fileId: 'unique()',
    file: InputFile(path: './test.file', filename: 'test.file'),
  );

When I update the docs, I will change the parameter description from Binary file. to File path string.
Screen Shot 2022-04-20 at 2 33 50 PM

We either have to make the description different for Dart (which I don't think is possible (?)) or we should change the current Dart SDK to also take a string.

@gewenyu99
Copy link
Author

gewenyu99 commented Apr 20, 2022

@TorstenDittmann See if these changes look good.

@gewenyu99 gewenyu99 changed the title [WIP] Fix File Create Docs Fix File Create Docs Apr 20, 2022
@lohanidamodar
Copy link
Member

@lohanidamodar I feel like the behavior of the Dart SDK isn't consistent with the other SDKs. It still takes an object like this:

  Future result = storage.createFile(
    bucketId: 'default',
    fileId: 'unique()',
    file: InputFile(path: './test.file', filename: 'test.file'),
  );

When I update the docs, I will change the parameter description from Binary file. to File path string. Screen Shot 2022-04-20 at 2 33 50 PM

We either have to make the description different for Dart (which I don't think is possible (?)) or we should change the current Dart SDK to also take a string.

We don't take path and instead use our own object, as we also accept MultiPart file, perks of Flutter web and Dart JS. So we cannot change this to accept file path. For now it's ok to leave the param description as it is, the file in the parameter list table should still be Binary file because we are not sending the Path in the HTTP request, and parameter table is for HTTP request, not SDK params. For SDK updating the examples should suffice, for dart/Flutter examples should already be updated.

Copy link
Contributor

@Meldiron Meldiron left a comment

Choose a reason for hiding this comment

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

Left a few comments. Can we also test all of these to make sure they are correct? We could test and document all regarding relative and absolute path.

@@ -127,7 +127,7 @@ public function getParamExample(array $param)
$output .= '{}';
break;
case self::TYPE_FILE:
$output .= "fs.createReadStream(__dirname + '/file.png')";
$output .= "__dirname + '/file.png'";
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be as simple as Deno, IIRC 'file.png'

@@ -146,11 +146,11 @@ public function getParamExample(array $param)
$output .= "'{$example}'";
break;
case self::TYPE_FILE:
$output .= "fs.createReadStream(__dirname + '/file.png')";
$output .= "__dirname + '/file.png'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -309,7 +309,7 @@ public function getParamExample(array $param)
$output .= '[]';
break;
case self::TYPE_FILE:
$output .= "new \CURLFile('/path/to/file.png', 'image/png', 'file.png')";
$output .= "'/path/to/file.png'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how absolute path would work. Can we keep this relative? Also, can we keep all examples consistant? We could do just 'file.png' everywhere

@@ -330,7 +330,7 @@ public function getParamExample(array $param)
$output .= "'{$example}'";
break;
case self::TYPE_FILE:
$output .= "new \CURLFile('/path/to/file.png', 'image/png', 'file.png')";
$output .= "'/path/to/file.png'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@gewenyu99
Copy link
Author

@Meldiron Sounds good. Let's standardize all the paths in examples. There's no reason to use a million different things

@gewenyu99
Copy link
Author

Left a few comments. Can we also test all of these to make sure they are correct? We could test and document all regarding relative and absolute path.

Yep I tested these examples manually. I will do them again with the the same type of path params (ideally use the same path parameters)

I'm going to be out until May 1st, so I'll get on it when I get back.

@gewenyu99 gewenyu99 requested a review from Meldiron May 2, 2022 23:18
@gewenyu99
Copy link
Author

@lohanidamodar I believe I've covered every SDK that now uses a path instead of a buffer.

There are several that did not change.

@christyjacob4 christyjacob4 merged commit d55028f into appwrite:master May 9, 2022
@electraport
Copy link

electraport commented Sep 14, 2022

Team, I hate to say it but your node.js documentation is still wrong. Your example on The AppWrite documentation uses the object you discuss as a Dart object above:

const promise = storage.createFile('[BUCKET_ID]', '[FILE_ID]', InputFile.fromPath('/path/to/file.png', 'file.png'));

But there is no class InputFile in Node. Thus, using the example code results in an undefined error wherever this line exists in my code.

a-good-filename.zip
/root/ep-util/unity.js:29
        const promise = storage.createFile( '<ProjectId>', id, InputFile.fromPath( path + file, file ) );
                                                                        ^

ReferenceError: InputFile is not defined
    at /root/folder/file.js:29:73
    at Array.forEach (<anonymous>)
    at /root/folder/file.js:25:11
    at FSReqCallback.oncomplete (fs.js:180:23)

In #32 the OP says it takes a string on line 332 of storage.js. I find that in the current version it is back to taking an InputFile:
* @param {InputFile} file

Linked Code, not sure how to make it preview like OP did.

There is nothing complex about my test code and in looking at the SDK it appears you define the model for InputFile and export it apparently properly but it's not working?

const sdk = require( 'node-appwrite' );
const fs = require( 'fs' );

// Init SDK
const client = new sdk.Client();

const storage = new sdk.Storage( client );

client
    .setEndpoint( 'https://my url/v1' ) // Your API Endpoint
    .setProject( 'my project ID' ) // Your project ID
    .setKey( '<my API Key' );

var path = '/opt/our-path/d01/';

//passsing directoryPath and callback function
fs.readdir( path, function ( err, files ) {
    //handling error
    if ( err ) {
        return console.log('Unable to scan directory: ' + err);
    }
    //listing all files using forEach
    files.forEach( function ( file ) {
        var id = 'unique()';
        // Do whatever you want to do with the file
        console.log( file );
        const promise = storage.createFile( '<Our-Project-ID>', id, InputFile.fromPath( path + file, file ) );

        promise.then( function ( response ) {
            console.log( response );
        }, function ( error ) {
            console.log( error );
        });
    });
});

fs.readdir returns a string array by default, so file should be a string and this should work if the compiler could find InputFile's definition.
But you should also change the documentation because it says it takes file which is type "file" which is "Binary file" which isn't true of type InputFile.
image

Any thoughts?

Thanks!

@electraport
Copy link

Team,

Further to the above it occurred to me I could do named imports from the package. So, a working snippet that should be along the lines of what's in documentation:

`const { Client, InputFile, Storage } = require( 'node-appwrite' );
const fs = require( 'fs' );

// Init SDK
const client = new Client();

const storage = new Storage( client );

client
.setEndpoint( 'https://<Your_Project_URL>/v1' ) // Your API Endpoint
.setProject( '<Your_Project_ID>' ) // Your project ID
.setKey( '' );

var path = '/your-path/';

//passsing directoryPath and callback function
fs.readdir( path, function ( err, files ) {
//handling error
if ( err ) {
return console.log('Unable to scan directory: ' + err);
}
//listing all files using forEach
files.forEach( function ( file ) {
var id = 'unique()';
// Do whatever you want to do with the file
console.log( file );
const promise = storage.createFile( '<Project_ID>', id, InputFile.fromPath( path + file, file ) );

    promise.then( function ( response ) {
        console.log( response );
    }, function ( error ) {
        console.log( error );
    });
});

});
`

Best regards,

EP

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.

5 participants