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

Method Annotations #585

Merged
merged 24 commits into from
Jan 9, 2023
Merged

Method Annotations #585

merged 24 commits into from
Jan 9, 2023

Conversation

briandowns
Copy link
Contributor

@briandowns briandowns commented Dec 28, 2022

Well detailed description of the change :

Initial pass at method annotation implementation. Method annotations have the same expectations as class annotations where they're not brought up from an inherited class.

This is a breaking change as the previous class field of annotations is now classAnnotations with an additional field methodAnnotations

Underlying class structure to carry the annotations:

// class annotations
{
    "ResourceController": "/api/v1"
}

// method annotations
{
    "methodName": {
        "Get": "/assets", 
        "BasicAuth": nil
    }
}

Resolves: #

#464

Type of change:

  • Bug fix

  • New feature

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Housekeeping

  • Tests have been updated to reflect the changes done within this PR (if applicable).

  • Documentation has been updated to reflect the changes done within this PR (if applicable).

Preview (Screenshots) :

@briandowns briandowns changed the title Issue 464 [WIP] - Method Annotations Dec 28, 2022
@briandowns briandowns marked this pull request as ready for review December 28, 2022 03:45
@briandowns
Copy link
Contributor Author

Might need some assistance @Jason2605 . All tests and examples are passing locally but not in CI and not sure wht the failure is there. Output seems to be cut off.

Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@Jason2605
Copy link
Member

Sorry @briandowns been away for the new year! Thanks for working on this, I haven't forgotten, I'll try to get around to seeing why the tests aren't passing

@Jason2605
Copy link
Member

Yeah for me running in the debug build im getting a segfault when running the annotation tests, will investigate further

```

Multiple annotations can be supplied to classes.
Method annotations are available on all class methods except initializers.
Copy link
Member

Choose a reason for hiding this comment

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

Whats the rationale for not on initialisers for my curiosity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most the thought of class initialization consistency. I'm happy to update it to allow for init to have annotations as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think allowing it on constructors could be useful, initial thoughts being it may be useful in the future (potentially?) if we were to create some sort of service container and enable some sort of clever dependency injection or something like that

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think allowing it on constructors could be useful, initial thoughts being it may be useful in the future (potentially?) if we were to create some sort of service container and enable some sort of clever dependency injection or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll work on this tonight. Thanks for all of the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

No worries, sorry it took a bit longer! Thanks for the effort on this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I think we're all set now.

docs/docs/classes.md Outdated Show resolved Hide resolved
src/vm/compiler.c Outdated Show resolved Hide resolved
src/vm/compiler.c Outdated Show resolved Hide resolved
src/vm/memory.c Outdated Show resolved Hide resolved
briandowns and others added 7 commits January 3, 2023 16:14
Co-authored-by: Jason_000 <Jason2605@users.noreply.github.com>
Co-authored-by: Jason_000 <Jason2605@users.noreply.github.com>
Co-authored-by: Jason_000 <Jason2605@users.noreply.github.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@Jason2605
Copy link
Member

Something I've noticed from a "correctness" standpoint is this is technically legal:

class Test {
    @Test
    @Test1
    @Test2
    @Test3
}

print(Test.methodAnnotations); // {"__annotatedMethodName": {"Test3": nil, "Test1": nil, "Test": nil, "Test2": nil}}

It is an edge case scenario but it would be good if this resulted in a compile time error since there's no method, what do you think?

Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@briandowns
Copy link
Contributor Author

I "think" we're all set.

@briandowns briandowns changed the title [WIP] - Method Annotations Method Annotations Jan 9, 2023
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@Jason2605
Copy link
Member

Legend, thank you very much for this!

@Jason2605 Jason2605 merged commit 8579de0 into dictu-lang:develop Jan 9, 2023
@briandowns
Copy link
Contributor Author

Thanks! Was a fun challenge. And thanks for the guidance and feedback through the process.

@Jason2605 Jason2605 mentioned this pull request Feb 17, 2023
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.

2 participants