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(modules): export module interfaces #932

Merged
merged 5 commits into from Sep 8, 2022
Merged

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented May 7, 2022

closes #778

@Shinigami92 Shinigami92 added the c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs label May 7, 2022
@Shinigami92 Shinigami92 self-assigned this May 7, 2022
@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label May 7, 2022
@Shinigami92 Shinigami92 requested a review from ST-DDT May 7, 2022 15:27
@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #932 (4b88870) into main (1fe2877) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2163     2163              
  Lines      241242   241264      +22     
  Branches     1014     1011       -3     
==========================================
+ Hits       240335   240354      +19     
- Misses        886      889       +3     
  Partials       21       21              
Impacted Files Coverage Δ
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/modules/address/index.ts 99.82% <100.00%> (ø)
src/modules/animal/index.ts 100.00% <100.00%> (ø)
src/modules/color/index.ts 99.73% <100.00%> (ø)
src/modules/commerce/index.ts 100.00% <100.00%> (ø)
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 96.24% <100.00%> (ø)
src/modules/date/index.ts 99.10% <100.00%> (-0.01%) ⬇️
... and 19 more

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label May 7, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

#805 (comment)

@Shinigami92
Copy link
Member Author

@pkuczynski Could you go through this PR/code and reevaluate if you REALLY want to have the modules renamed not to suffixed with Module? I still don't think your initial thought that it could conflict with node's modules is valid in any case.
Beside that, I personally find it confusing to name modules everywhere modules but then the module itself is not named module but faker were it actually is not e.g. a sub-instance of Faker.
It can be even more confusing if we have later a MinimalFaker, BaseFaker, Faker. (See #805 (reply in thread))

And then we would even have a FakeFaker 👀 confusing like hell 🤷

@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

not to suffixed with Module?

I think so too, it should be named like what it does and not what generically is.
Also in the long run, a module will never be independent, a xxxFaker might.

And then we would even have a FakeFaker 👀 confusing like hell 🤷

No, because fake -> helpers.fake

@Shinigami92 Shinigami92 added the breaking change Cannot be merged when next version is not a major release label May 23, 2022
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Aug 13, 2022
@Shinigami92 Shinigami92 changed the title refactor: rename module class names refactor(modules)!: rename class names Aug 13, 2022
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label Aug 13, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review August 20, 2022 10:36
@Shinigami92 Shinigami92 requested a review from a team as a code owner August 20, 2022 10:36
src/faker.ts Show resolved Hide resolved
@xDivisionByZerox
Copy link
Member

Would it be possible to integrate this change as a non-breaking change? Maybe with some proxy magic and deprecation of the current classes (in the constructor)?

@ST-DDT
Copy link
Member

ST-DDT commented Aug 21, 2022

Would it be possible to integrate this change as a non-breaking change? Maybe with some proxy magic and deprecation of the current classes (in the constructor)?

The classes have never been part of the API, so not sure if that is worth it.

@Shinigami92 Shinigami92 changed the title refactor(modules)!: rename class names feat(modules)!: export module interfaces and suffix them with Module Sep 7, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2022

Team decision

We will use XyzModule.

@Shinigami92 Shinigami92 removed the s: needs decision Needs team/maintainer decision label Sep 8, 2022
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed breaking change Cannot be merged when next version is not a major release labels Sep 8, 2022
@Shinigami92 Shinigami92 changed the title feat(modules)!: export module interfaces and suffix them with Module feat(modules): export module interfaces and suffix them with Module Sep 8, 2022
@ST-DDT ST-DDT changed the title feat(modules): export module interfaces and suffix them with Module feat(modules): export module interfaces Sep 8, 2022
@ST-DDT ST-DDT enabled auto-merge (squash) September 8, 2022 17:11
@ST-DDT ST-DDT merged commit b9884d0 into main Sep 8, 2022
@ST-DDT ST-DDT deleted the rename-module-classes branch September 8, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cannot directly import _Date
5 participants