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

add method declaration support #125

Merged
merged 7 commits into from
Oct 21, 2024
Merged

add method declaration support #125

merged 7 commits into from
Oct 21, 2024

Conversation

OlegDokuka
Copy link
Member

@OlegDokuka OlegDokuka commented Oct 8, 2024

This PR maps ts.MethodDeclaration into J.MethodDeclaration. The only custom JS LST type it adds is JS.TypeInfo, which is a helper type to keep info about spaces before : token (in js type info is defined as e.g. var a : String = "asa so we need an extra container to keep spaces before and after : sign)


openrewrite/package.json Show resolved Hide resolved
openrewrite/src/javascript/parser.ts Outdated Show resolved Hide resolved
openrewrite/src/javascript/parser.ts Outdated Show resolved Hide resolved
);
});

test('multiple type parameters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

A test with a return type would also be good.

@OlegDokuka OlegDokuka force-pushed the main-method-declaration branch 2 times, most recently from 2d69922 to cb8c868 Compare October 16, 2024 22:21
[],
[],
this.visit(node.name),
null
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Adding the constraints can be done separately. Unlike Java I think TS allows specifying anonymous types inline as in:

type Animal = { name: string };
type Dog = { breed: string } & Animal;

function handleAnimal<T extends Animal>(animal: T) {
    console.log(animal.name);
}

function handleAnimals<T extends { breed: string } & Animal>(animal: T) {
    console.log(animal.name);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

yup. and for now idk how to solve that properly

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the parsing will not be an issue, ad the property is of type TypeTree. We just need to implement the mapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, ok

@With
public class TypeReferencePrefix implements Marker {
UUID id;
Space prefix;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what we should do about this. I know that this is how this was done for Kotlin, but we specifically want to avoid adding any Space properties to markers.

I also haven't yet decided how we should address this in Python, where I for now "inherited" the solution that was implemented there, which was to wrap the return type into a TypeHint LST type, which then contains that additional space as its prefix. Yet another option would be to wrap the J.MethodDeclaration.

None of these options seem quite right to me.

Can you raise this question on the Slack channel and then once we've settled on something we can update this code. Since we really want to avoid Space properties in markers, I suspect we will end up changing this, however.

@OlegDokuka OlegDokuka force-pushed the main-method-declaration branch from b5ce401 to ca71be3 Compare October 21, 2024 11:27
@arodionov arodionov self-requested a review October 21, 2024 11:28
Signed-off-by: OlegDokuka <[email protected]>
@OlegDokuka OlegDokuka merged commit 9895db1 into main Oct 21, 2024
2 checks passed
@OlegDokuka OlegDokuka deleted the main-method-declaration branch October 21, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants