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

Test renamed imports #870

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

Conversation

MatthiasKunnen
Copy link
Contributor

tsoa does not support renaming of decorators in imports nor renaming of models. This is a problem in case there is a naming conflict, for example when you have a model named Post.

The two new fixtures included in this PR contain examples of decorators such as Get being renamed and models being renamed.

Model renaming crashes the test suite outright. Method decorator renaming fails more gracefully.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Jan 14, 2021
@MatthiasKunnen
Copy link
Contributor Author

Not stale

@github-actions github-actions bot removed the Stale label Jan 15, 2021
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Feb 15, 2021
@MatthiasKunnen
Copy link
Contributor Author

MatthiasKunnen commented Feb 15, 2021 via email

@github-actions github-actions bot removed the Stale label Feb 16, 2021
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Mar 18, 2021
@WoH WoH removed the Stale label Mar 18, 2021
@hasyimibhar
Copy link

hasyimibhar commented Mar 30, 2021

Is there a plan to fix this? I think it fails because tsoa checks the name of the decorator. Instead of doing that, maybe the decorator should attach a metadata to the class or method. Then tsoa can check for the presence of the metadata instead of checking for the name of the decorator. This will make it easier to integrate tsoa to existing framework which also use decorators, as you can then compose decorators.

For example, in the project I'm working on, I'm using inversify-express-utils, which allows dependencies to be injected into controller methods. But because this injected dependency will break tsoa, I have to make sure it's ignored with the @Inject decorator. So my code looks like this:

class FoobarController {
  @Post('/')
  foobar(
    // Looks ugly
    @Inject() @Inject(TYPES.User) user: User,
    @Body() req: FoobarRequest
  ) {
    // ...
  }
}

If tsoa use the metadata approach, then I can create a custom decorator in my project that calls both of these decorators, so now it can look like this:

class FoobarController {
  @Post('/')
  foobar(
    // InjectUser() is wrapper for calling both @Inject() and @Inject(TYPES.User)
    @InjectUser() user: User,
    @Body() req: FoobarRequest
  ) {
    // ...
  }
}

This will also fix the naming issue, as the user is now free to rename the decorators to anything when importing, as tsoa no longer depends on the name of the decorator.

I'm not that familiar with tsoa codebase or it this would even work (so far I only made a small PR to add the @Inject decorator), so I'm not sure if this is difficult to implement. But if it's not too difficult, and the approach makes sense, maybe I can offer a hand.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Apr 30, 2021
@MatthiasKunnen
Copy link
Contributor Author

MatthiasKunnen commented Apr 30, 2021 via email

@WoH
Copy link
Collaborator

WoH commented May 13, 2021

Is there a plan to fix this?

Probably not. At least I'm not aware of any easy solution.

I think it fails because tsoa checks the name of the decorator. Instead of doing that, maybe the decorator should attach a metadata to the class or method.

We can do that to make the runtime work, but we can't access that metadata when we compile the Spec

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Jun 13, 2021
@MatthiasKunnen
Copy link
Contributor Author

MatthiasKunnen commented Jun 13, 2021 via email

@github-actions github-actions bot removed the Stale label Jun 14, 2021
@WoH
Copy link
Collaborator

WoH commented Jun 15, 2021

No keep open option?

On Sun, Jun 13, 2021, 02:28 github-actions[bot] @.***> wrote: This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#870 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEAHNI6DO5EHC5TBRWB3RMLTSP3TXANCNFSM4U3KRSUQ .

The help wanted label should prevent that, I'll take a look and reopen manually if it happens again

@WoH WoH reopened this Jul 22, 2021
Repository owner deleted a comment from github-actions bot Jul 22, 2021
@WoH WoH removed the Stale label Jul 22, 2021
@github-actions github-actions bot added the Stale label Aug 22, 2021
@WoH WoH removed the Stale label Aug 23, 2021
Repository owner deleted a comment from github-actions bot Aug 23, 2021
@WoH
Copy link
Collaborator

WoH commented Jan 30, 2023

This is now partially solved (model renaming). As for Decorators, this is not really a prio for me.

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.

None yet

3 participants