Skip to content
This repository was archived by the owner on Sep 29, 2023. It is now read-only.

Conversation

adityaoberai
Copy link
Member

@adityaoberai adityaoberai commented Jun 6, 2022

Adds a .NET, Kotlin, and Java function examples to the Writing your own Function section in the Functions guide.

@adityaoberai adityaoberai changed the title Add .NET function example to Functions Guide Add .NET, Kotlin function example to Functions Guide Jun 6, 2022
@adityaoberai adityaoberai changed the title Add .NET, Kotlin function example to Functions Guide Add .NET, Kotlin, Java function examples to Functions Guide Jun 6, 2022
@Meldiron
Copy link
Contributor

Meldiron commented Jun 7, 2022

Screenshots:

CleanShot 2022-06-07 at 12 32 05

CleanShot 2022-06-07 at 12 32 01

CleanShot 2022-06-07 at 12 31 54

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.

After applying these, please double-check if they still work. We might have a bug in open-runtimes if these changes don't work. (payload should always default to empty string)

@gewenyu99
Copy link
Contributor

I think this looks alright, I have no comments on the code itself, mainly because I don't know kotlin or C# well. I would think adding some comments might be useful.

It depends on the language, but we should at least comment on what the function is supposed to do in a comment.

Copy link
Contributor

@gewenyu99 gewenyu99 left a comment

Choose a reason for hiding this comment

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

I would suggest adding some comments so the code is easier to follow. I say this mostly because I don't know these languages very well, and I subconsciously started looking for a comment.

@adityaoberai
Copy link
Member Author

I would suggest adding some comments so the code is easier to follow. I say this mostly because I don't know these languages very well, and I subconsciously started looking for a comment.

That's actually not been across any of the samples. Maybe something we should have across them all?

adityaoberai and others added 4 commits June 10, 2022 15:12
Co-authored-by: Matej Bačo <matejbaco2000@gmail.com>
Co-authored-by: Matej Bačo <matejbaco2000@gmail.com>
Co-authored-by: Matej Bačo <matejbaco2000@gmail.com>
@adityaoberai
Copy link
Member Author

adityaoberai commented Jun 10, 2022

@Meldiron I removed the check for {} with the check for a null payload and got this in each case:

  • .NET

image

  • Java

image

  • Kotlin

image

Could this be the Console sending {} as a default payload if left empty?

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.

LGTM

@Meldiron
Copy link
Contributor

@adityaoberai don't worry about '{}' payload, it is a bug in Appwrite. Addressed in this PR: appwrite/appwrite#3367

Payload (should, and) will be empty string if not provided.

@adityaoberai
Copy link
Member Author

Sure 👍
I have updated the function examples to check the payload for an empty string only.

@adityaoberai
Copy link
Member Author

I think this looks alright, I have no comments on the code itself, mainly because I don't know kotlin or C# well. I would think adding some comments might be useful.

It depends on the language, but we should at least comment on what the function is supposed to do in a comment.

@gewenyu99 maybe this could be an entirely separate issue of it's own that we could work on? I'll go ahead and create one in the meantime.

}</code></pre>
</div>
<p><b>Installing Dependencies</b></p>
<p>Include a <b>Function.csproj</b> file in your function, Appwrite handles the rest!</p>
Copy link
Contributor

@christyjacob4 christyjacob4 Jun 13, 2022

Choose a reason for hiding this comment

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

Suggested change
<p>Include a <b>Function.csproj</b> file in your function, Appwrite handles the rest!</p>
<p>Include a <b>Function.csproj</b> file with your function - Appwrite handles the rest!</p>

I saw that some of the existing functions also use in and a comma.
Please update those as well

Copy link
Member Author

@adityaoberai adityaoberai Jun 13, 2022

Choose a reason for hiding this comment

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

Will do that 👍
I was following suit, to be honest.

}</code></pre>
</div>
<p><b>Installing Dependencies</b></p>
<p>Include a <b>deps.gradle</b> file in your function, Appwrite handles the rest!</p>
Copy link
Contributor

@christyjacob4 christyjacob4 Jun 13, 2022

Choose a reason for hiding this comment

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

Suggested change
<p>Include a <b>deps.gradle</b> file in your function, Appwrite handles the rest!</p>
<p>Include a <b>deps.gradle</b> file with your function - Appwrite handles the rest!</p>

Likewise

}</code></pre>
</div>
<p><b>Installing Dependencies</b></p>
<p>Include a <b>deps.gradle</b> file in your function, Appwrite handles the rest!</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>Include a <b>deps.gradle</b> file in your function, Appwrite handles the rest!</p>
<p>Include a <b>deps.gradle</b> file in your function - Appwrite handles the rest!</p>

Copy link
Contributor

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Please address the comments

@adityaoberai
Copy link
Member Author

@christyjacob4 made the grammatical (+ a couple of other minor ones I saw)

@christyjacob4 christyjacob4 merged commit 2554fa6 into appwrite:main Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants