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(common): define type for file input #11040

Merged

Conversation

mohamadrezamomeni
Copy link
Contributor

@mohamadrezamomeni mohamadrezamomeni commented Feb 3, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

coveralls commented Feb 3, 2023

Pull Request Test Coverage Report for Build ed678257-d3f2-408a-b523-68e1532a63e1

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • 29 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.03%) to 92.954%

Files with Coverage Reduction New Missed Lines %
packages/core/inspector/deterministic-uuid-registry.ts 1 75.0%
packages/core/router/paths-explorer.ts 1 91.3%
packages/websockets/exceptions/base-ws-exception-filter.ts 1 92.86%
packages/websockets/gateway-metadata-explorer.ts 1 88.46%
packages/microservices/listener-metadata-explorer.ts 2 87.1%
packages/core/injector/instance-wrapper.ts 3 93.93%
packages/common/pipes/validation.pipe.ts 4 90.22%
packages/core/scanner.ts 16 87.17%
Totals Coverage Status
Change from base Build 87b1d40c-8c24-4710-8c62-a3137e8d2971: 0.03%
Covered Lines: 6412
Relevant Lines: 6898

💛 - Coveralls

@mohamadrezamomeni
Copy link
Contributor Author

define type for file details input in pipes

@micalevisk
Copy link
Member

micalevisk commented Feb 3, 2023

as FileTypeValidator is a public API, this PR introduces breaking changes.

@mohamadrezamomeni
Copy link
Contributor Author

as FileTypeValidator is a public API, this PR introduces breaking changes.

thank you for comment.

@mohamadrezamomeni
Copy link
Contributor Author

as FileTypeValidator is a public API, this PR introduces breaking changes.
i want to change any type for file to specific interface. then write unit test based on this interface. is it acceptable?

@mohamadrezamomeni mohamadrezamomeni changed the title Refactor define type refactor(common): define type for file input Feb 4, 2023
@kamilmysliwiec
Copy link
Member

@thiagomini want to take a look? 👀

@micalevisk
Copy link
Member

@mohamadrezamomeni sounds good

Copy link
Contributor

@thiagomini thiagomini left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I think we only need to adjust a few things, tho. Check my suggestions below.

packages/common/pipes/file/interfaces/file.interface.ts Outdated Show resolved Hide resolved
packages/common/pipes/file/file-validator.interface.ts Outdated Show resolved Hide resolved
Co-authored-by: Thiago Valentim <51245643+thiagomini@users.noreply.github.com>
@mohamadrezamomeni
Copy link
Contributor Author

@thiagomini what is your opinion right now ? :)

@thiagomini
Copy link
Contributor

Hey, man, I'll review this PR again tomorrow. The last two days have been quite busy around here 😅. Thanks again for the contribution.

Copy link
Contributor

@thiagomini thiagomini left a comment

Choose a reason for hiding this comment

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

This LGTM now, but there's only one question left: why did you add the || { } in the generic type?

@mohamadrezamomeni
Copy link
Contributor Author

mohamadrezamomeni commented Feb 9, 2023

This LGTM now, but there's only one question left: why did you add the || { } in the generic type?

It is issue i put || { } because according to the test input of isValue may be empty object. can you guide me what to do for this problem? i wanted to use // // @ts-ignore in unit test but i don't know it is good idea or no

@thiagomini
Copy link
Contributor

This LGTM now, but there's only one question left: why did you add the || { } in the generic type?

It is issue i put || { } because according to the test input of isValue may be empty object. can you guide me what to do for this problem? i wanted to use // // @ts-ignore in unit test but i don't know it is good idea or no

I think we don't have to be explicit about that. The any case should already cover it.

@mohamadrezamomeni
Copy link
Contributor Author

I think we don't have to be explicit about that. The any case should already cover it.

do it :)

@mohamadrezamomeni
Copy link
Contributor Author

@kamilmysliwiec do you have any opinion right now?

@kamilmysliwiec
Copy link
Member

@mohamadrezamomeni this is a breaking change so we must wait till the next major release

@mohamadrezamomeni mohamadrezamomeni changed the title refactor(common): define type for file input feat(common): define type for file input Mar 10, 2023
@kamilmysliwiec kamilmysliwiec added this to the 10.0.0 milestone Apr 5, 2023
@kamilmysliwiec kamilmysliwiec changed the base branch from master to 10.0.0 April 5, 2023 11:07
@kamilmysliwiec kamilmysliwiec merged commit 2eb932c into nestjs:10.0.0 Apr 5, 2023
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.

5 participants