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

feat: added constants for Error Types #724

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

35C4n0r
Copy link

@35C4n0r 35C4n0r commented Oct 10, 2023

What does this PR do?

  • added logic in Swagger2.php to parse errors from APISpecs -> definitions->x-appwrite.
  • added Error enums in twig templates.

Related PRs and Issues

Closes #698

- added logic in Swagger2.php to parse errors from APISpecs -> definitions->x-appwrite.
- added Error enums in twig templates.

Signed-off-by: Jay <[email protected]>
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Wow, this is amazing! Would you be able to add test cases as well? For reference, here's a PR with tests for Flutter/Dart.

@35C4n0r
Copy link
Author

35C4n0r commented Oct 21, 2023

Wow, this is amazing! Would you be able to add test cases as well? For reference, here's a PR with tests for Flutter/Dart.

This is what the generate enums will look like (Python code):

class ErrorType(Enum):
    """
    General errors thrown by the mock controller used for testing.
    """
    GeneralMock = 'general_mock'

    """
    The request contains one or more invalid arguments. Please refer to the endpoint documentation.
    """
    GeneralArgumentInvalid = 'general_argument_invalid'

@stnguyen90 what kind of tests would we want for the generated enums,
also I think someone is working on adding the testcases here #680

@stnguyen90
Copy link
Contributor

@35C4n0r

what kind of tests would we want for the generated enums,

tests comparing the enum to the string value like:

ErrorType.GeneralMock == 'general_mock`

This is exactly how the enums will be used by developers using the SDK.

also I think someone is working on adding the testcases here #680

Ya but they wouldn't be able to add tests for this because the enum code is in your PR.

@mvarendorff
Copy link
Contributor

mvarendorff commented Oct 21, 2023

Heyyo :) Guy working on the tests here! I am only just getting started, and the tests are a bit of a pain to set up from the ground up. So if this is merged, I don't mind rebasing my stuff and potentially including them.

Only thing that would help me would be a reference testcase or two in the dart / flutter SDK so I can port that to the other SDKs. I am currently on the Android front still and it's going to be some time until I get my hands on other SDKs.

Just offering this up because adding testcases across all SDKs is otherwise going to take a bit and probably cause more chaos merging things than if this was merged and I just included the tests in my work.

Thoughts @stnguyen90 ?

- added new test folder in python.
- added test for Python ErrorEnums

Signed-off-by: Jay <[email protected]>
@35C4n0r
Copy link
Author

35C4n0r commented Oct 21, 2023

@stnguyen90 This is what I came up with, 061fea8, if it looks good i'll add for the rest of the sdks.

@stnguyen90 stnguyen90 self-requested a review October 23, 2023 16:18
@stnguyen90
Copy link
Contributor

@geisterfurz007, @35C4n0r can add these test cases since they should be self contained and shouldn't impact the other tests too much.

@stnguyen90
Copy link
Contributor

@35C4n0r, btw, best practices for commit messages is to use imperative (i.e. use add rather than added). For more info, see https://cbea.ms/git-commit/

- add new gem test-unit

Signed-off-by: Jay <[email protected]>
@35C4n0r
Copy link
Author

35C4n0r commented Oct 25, 2023

@geisterfurz007 what testing library are you planning to use for node runtime.
For now I'm using assert.strictEqual(), but let me know.

@mvarendorff
Copy link
Contributor

@35C4n0r Whatever works for you for now is good, I think. So far I am only done-ish with Kotlin where I used kotlin-test / JUnit. Everything else is still pending for me. Should I pick another library anywhere for any good reason, I'd probably just migrate these tests over.

- new tests for Node, Deno, web, swift, ruby, python, php, kotlin, dotnet, android

Signed-off-by: Jay <[email protected]>
- new tests for Dart and Flutter

Signed-off-by: Jay <[email protected]>
@35C4n0r
Copy link
Author

35C4n0r commented Oct 25, 2023

@stnguyen90 I've added the Tests, PR is up for review.

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great PR! 🤯 We left some comments during the review, please check them out.

src/SDK/SDK.php Outdated Show resolved Hide resolved
src/Spec/Swagger2.php Outdated Show resolved Hide resolved
templates/android/tests/TestException.kt.twig Outdated Show resolved Hide resolved
src/Spec/Swagger2.php Outdated Show resolved Hide resolved
src/Spec/Swagger2.php Outdated Show resolved Hide resolved
@35C4n0r
Copy link
Author

35C4n0r commented Nov 4, 2023

@stnguyen90 I have made changes according to the comments. Kindly re-review

@lohanidamodar
Copy link
Member

@35C4n0r Thank you for update to the PR however, I'm still seeing so many tests fail, will you be able to investigate why?

@35C4n0r
Copy link
Author

35C4n0r commented Nov 12, 2023

@lohanidamodar @stnguyen90 There were a minor issues that i've fixed.

This is what i've added in my spec to test in the local.

    "appwriteException": {
      "properties": {
        "message": {
          "type": "string",
          "description": "Error message.",
          "x-example": "Invalid id: Parameter must be a valid number"
        },
        "type": {
          "type": "string",
          "description": "Error type.",
          "enum": [
            "general_mock",
            "general_argument_invalid"
          ],
          "x-example": "argument_invalid"
        },
        "code": {
          "type": "integer",
          "description": "Error code.",
          "x-example": 400,
          "format": "int32"
        }
      },
      "x-appwrite": {
        "types": [
          {
            "code": 400,
            "type": "general_mock",
            "message": "General errors thrown by the mock controller used for testing."
          },
          {
            "code": 400,
            "type": "general_argument_invalid",
            "message": "The request contains one or more invalid arguments. Please refer to the endpoint documentation."
          }
        ]
      }

Now, the remaining tests for Python, Dart and Java are failing, cause the spec passed to the SDK doesn't have any error types (under x-appwrite) present in them, in which case no enum element is generated, what would be a better way to handle this would be to not generate the ErrorType enum if there are 0 elements. Also this issue is arising as the spec is not updated with the error types.

@35C4n0r
Copy link
Author

35C4n0r commented Nov 14, 2023

@lohanidamodar @stnguyen90 any thoughts on it?

@stnguyen90
Copy link
Contributor

what would be a better way to handle this would be to not generate the ErrorType enum if there are 0 elements.

Ya, let's be defensive and handle the case so things don't break.

Also this issue is arising as the spec is not updated with the error types.

The specs used in the tests are here. They should probably be updated to include the x-appwrite stuff.

@35C4n0r
Copy link
Author

35C4n0r commented Nov 17, 2023

@stnguyen90 I have changed the test Spec to this https://github.com/appwrite/sdk-generator/blob/c429aa0432aa70c3e987106b7e8bb42d12b4a903/tests/resources/spec.json

There is still an issue where AppwrwiteException Class is already written in the src/exceptions.* and is again bieng generated in the Models/. because of this

"appwriteException": {

which is leading to ambiguos naming and import errors, see: https://github.com/appwrite/sdk-generator/actions/runs/6901109358/job/18775353731?pr=724#step:8:819, https://github.com/appwrite/sdk-generator/actions/runs/6901109358/job/18775352926?pr=724#step:8:248

How should we fix this.

@stnguyen90
Copy link
Contributor

How should we fix this?

Can we have an exclude to exclude creating models for things that end in Exception?

@35C4n0r
Copy link
Author

35C4n0r commented Nov 22, 2023

@stnguyen90 @lohanidamodar all tests pass, the PR is ready for review.

@lohanidamodar
Copy link
Member

@35C4n0r Awesome work, will work to get this merged as soon as possible.

@gewenyu99
Copy link

Hey there! There were a lot of big PRs during this Hacktoberfest, and we wanted to give everyone ample time to collaborate with our engineering team. If you were able to merge your PRs during October, amazing. If it’s still not merged, don’t worry about it either. Either way, we’ve got your Hacktoberfest swag minted and ready to ship.

Please comment with your Discord username here so we can contact you about your shipping information to deliver your Hacktoberfest swag.

@35C4n0r
Copy link
Author

35C4n0r commented Apr 9, 2024

Hey there! There were a lot of big PRs during this Hacktoberfest, and we wanted to give everyone ample time to collaborate with our engineering team. If you were able to merge your PRs during October, amazing. If it’s still not merged, don’t worry about it either. Either way, we’ve got your Hacktoberfest swag minted and ready to ship.

Please comment with your Discord username here so we can contact you about your shipping information to deliver your Hacktoberfest swag.

@gewenyu99, Here is my DiscordID: huntersoulz

@gewenyu99
Copy link

Reaching out soon!

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.

🚀 Feature: Constants for Error Types
5 participants