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

chore: rename generics #2046

Merged
merged 2 commits into from May 4, 2023
Merged

chore: rename generics #2046

merged 2 commits into from May 4, 2023

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 14, 2023

This PR renamed generics to a convention suggested by one of the TypeScript experts @mattpocock based on the YouTube video: https://www.youtube.com/watch?v=qA65QjWCl60

Also based on this poll (https://main.elk.zone/mas.to/@jvhellemond@mastodon.social/110124086297164650) by @jvhellemond entries are renamed to elements when it is in the context of an array/set extracted into #2049

I only checked src folder

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Apr 14, 2023
@Shinigami92 Shinigami92 self-assigned this Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #2046 (a25ba9e) into next (0740aa0) will decrease coverage by 0.01%.
The diff coverage is 74.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2046      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2605     2605              
  Lines      244951   244951              
  Branches     1257     1256       -1     
==========================================
- Hits       243979   243974       -5     
- Misses        945      950       +5     
  Partials       27       27              
Impacted Files Coverage Δ
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/utils/types.ts 0.00% <0.00%> (ø)
src/locale-proxy.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 99.09% <100.00%> (ø)
src/modules/helpers/unique.ts 98.70% <100.00%> (ø)

... and 2 files with indirect coverage changes

@Shinigami92 Shinigami92 marked this pull request as ready for review April 14, 2023 08:18
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 14, 2023 08:18
@Shinigami92 Shinigami92 requested a review from ST-DDT April 14, 2023 08:19
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I'm really torn on this one.

  • I like using useful names
  • I don't like the TName convention
  • I don't think it adds value for methods that only do T* -> T
  • I think TValue/Value does not transport any additional meaning over T

src/definitions/definitions.ts Outdated Show resolved Hide resolved
src/definitions/definitions.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision p: 1-normal Nothing urgent labels Apr 14, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

I watched the video and he explicitly mentions that T is fine as long as you don't have multiple (RSTUV...) ones.

As for TPrefixed: I see what argument he is trying to make, but if you cannot remember that NotPrefixed is your generic, then there are probably more important issues such as bad naming or a too complex method.

@Shinigami92
Copy link
Member Author

Why might need to activate https://typescript-eslint.io/rules/naming-convention/#enforce-that-type-parameters-generics-are-prefixed-with-t

@matthewmayer
Copy link
Contributor

matthewmayer commented Apr 14, 2023

T is prefixed with T :)

I feel this is unnecessary. <T> is very commonly understood in both Typescript and from other languages like Java. If i see <TRecord> i wonder if you are doing something special and different.

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

I sadly can't reject the only request changes. Can I request to change everything back?
All of this is overkill. There are good situations to get verbose with your generic names. The ones changed in this PR do not belong to that category.

@Shinigami92
Copy link
Member Author

I'm really torn on this one.

  • I like using useful names

Me either 👍
Even while working on this PR, I now more and more respect this naming convention and I'm tempted to use it for all my projects, because you can directly and easily read what the generic type is aimed for instead of first reading the code and try to find it out.

  • I don't like the TName convention

Do you have a reason for that?

  • I don't think it adds value for methods that only do T* -> T

We have one example for when T is the result/return value of a function and one for when it is for the input.
Now with this change it is directly readable from the code for what the generic type is used for.

Yeah okay we could discuss this specific case and maybe just use better names from the start like Input and Result, but the T prefix makes it better in two ways:

  1. it does not conflict with other types like if you would have an interface called Result (for what ever reason)
  2. you can directly find the generic types vs the other types/interfaces used in "more complex" code
  • I think TValue/Value does not transport any additional meaning over T

read above


T is prefixed with T :)

I feel this is unnecessary. <T> is very commonly understood in both Typescript and from other languages like Java. If i see <TRecord> i wonder if you are doing something special and different.

  1. I'm not tempted to follow old ways of doing something just because it was ever done like that without arguments for pro or con
  2. Yes, maybe we need to find a better name instead of TRecord, and @ST-DDT already pointed to that: chore: rename generics #2046 (comment)

I sadly can't reject the only request changes. Can I request to change everything back? All of this is overkill. There are good situations to get verbose with your generic names. The ones changed in this PR do not belong to that category.

These are really very generic (pun not intended) reasons without any real valuable arguments 😕
You say There are good situations to get verbose with your generic names but doesn't come up with an example but just the statement.
Please substantiate your statement.

scripts/generateLocales.ts Outdated Show resolved Hide resolved
src/definitions/definitions.ts Outdated Show resolved Hide resolved
src/internal/merge.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member Author

Addition: This PR aims to reduce the cognitive load of reading the code / using the methods

@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

  • I don't like the TName convention

Do you have a reason for that?

Sure, it creates an arbitrary rule to name something in a special way inconsistently.
I could flip this around and ask why don't you prefix both types and interfaces with I?
What makes a method local type different from a file local (not exported) one or exported ones?

EDIT:

You could even use G as prefix, because it refers to a Generic type.
Using T is part of that very convention that you try to abolish.

EDIT2:

"prefix both types and interfaces with I" to avoid conflicts during type <-> interface refactors.
If the distinction is important, than a separate prefix would be as well.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

but the T prefix makes it better in two ways:

  1. it does not conflict with other types like if you would have an interface called Result (for what ever reason)

But what if you have an exported type TResult "for what ever reason"? Ambiguous naming is prone to collisions.

  1. you can directly find the generic types vs the other types/interfaces used in "more complex" code

If the generic type is simple you don't need a full name in the first place.
Both generics and external code can get pretty complex, I often put this complexity into the methods generics, so that TS resolves the result instead of displaying Whatever<T>. But it is very easy to outsource the generic complexity to a separate type. And if I can outsource or inline it, why would I name it specially?

@xDivisionByZerox
Copy link
Member

I could flip this around and ask why don't you prefix both types and interfaces with I?

This! Excatly this!!!
Why stop there use the correct prefix for Interfaces and Types.

export interface IMyInterface {
  // ... definition
}

export type TMyType = // definition

I mean it cool, no? You just have to type I or T and VSCode suggests all possible interfaces or types.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

I feel this is unnecessary. <T> is very commonly understood in both Typescript and from other languages like Java. If i see <TRecord> i wonder if you are doing something special and different.

  1. I'm not tempted to follow old ways of doing something just because it was ever done like that without arguments for pro or con

I'm not tempted to drop proven ways of doing something just because of a single video without arguments for pro or con.

Pro simple generics:

  • It is a well understood concept
  • It doesn't bloat the code, with TThisIsSomethingReallyUnimportantName over "hey this is something dynamic called T".
  • It doesn't require us to teach every new contributor, that we have this special rule for generics.
  1. Yes, maybe we need to find a better name instead of TRecord, and @ST-DDT already pointed to that:

If you have a suggestion, we can discuss it. Currently the increased length of the name only takes more time to read till the extends clause that already tells me it is a Record. And if you try to guess what a method does without at least having a look at the method name/definition is stupid. If the method or generic is poorly named, then a longer name won't help either.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

I sadly can't reject the only request changes. Can I request to change everything back? All of this is overkill. There are good situations to get verbose with your generic names. The ones changed in this PR do not belong to that category.

These are really very generic (pun not intended) reasons without any real valuable arguments 😕 You say There are good situations to get verbose with your generic names but doesn't come up with an example but just the statement. Please substantiate your statement.

Yes, the reasoning is a bit fussy, but IMO it is a valid point.
If you want a reason: I assign specific names if the name carries value, not just because I can.

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Apr 14, 2023

These are really very generic (pun not intended) reasons without any real valuable arguments 😕
You say There are good situations to get verbose with your generic names but doesn't come up with an example but just the statement.
Please substantiate your statement.

Sure. Have a look at the implementation of a JS Generator:

interface Generator<T = unknown, TReturn = any, TNext = unknown> extends Iterator<T, TReturn, TNext> {
    // NOTE: 'next' is defined using a tuple to ensure we report the correct assignability errors in all places.
    next(...args: [] | [TNext]): IteratorResult<T, TReturn>;
    return(value: TReturn): IteratorResult<T, TReturn>;
    throw(e: any): IteratorResult<T, TReturn>;
    [Symbol.iterator](): Generator<T, TReturn, TNext>;
}

This makes to have verbose names as we have multiple generics.

Again, as long as you don't have multiple generic values you don't need to get creative. Please check the video again (time 1:59min to 2:01min):

For simple types like this I will generally keep this as the single letter T

@xDivisionByZerox
Copy link
Member

@mattpocock May you give me/us some insight if this PR is what you wanted to achieve when creating your video How to Name your Types?

@ST-DDT
Copy link
Member

ST-DDT commented Apr 14, 2023

Addition: This PR aims to reduce the cognitive load of reading the code / using the methods

These are really very generic [...] reasons without any real valuable arguments 😕 You say [...] but doesn't come up with an example but just the statement. Please substantiate your statement.

No seriously: Reading more text for the same amount of information usually doesn't "reduce the cognitive load".

@Shinigami92
Copy link
Member Author

I would like to remember @xDivisionByZerox and @ST-DDT to read our Code of Conduct again.

In the interest of fostering an open and welcoming environment, we as contributors and maintainers pledge to making participation in our project and our community a harassment-free experience for everyone, regardless of age, body size, disability, ethnicity, gender identity and expression, level of experience, education, socio-economic status, nationality, personal appearance, race, religion, or sexual identity and orientation.

Please don't blame me down or make assumptions about my potential level of experience.

Examples of behavior that contributes to creating a positive environment
include:
- Using welcoming and inclusive language
- Being respectful of differing viewpoints and experiences

Please try to write a bit more respectful comments especially in terms of differing viewpoints and experiences.
Using welcoming and inclusive language can also help finding a better path to a more constructive base.

I call cap. If you use a higher order type and don't know what the generic is supposed to be that's bad... like from your side, and you should probably not work with those types in that case.

(#2046 (comment))

For example we should prevent using exclamations like I call crap and just leave it out of the discussion.
Furthermore we could write with more welcoming and inclusive language like:

- I call cap. If you use a higher order type and don't know what the generic is supposed to be that's bad... like from your side, and you should probably not work with those types in that case.
+ If you use a higher order type, the generic template parameter is only supposed to transfer the given type the user is providing through it. However we should keep care on your side to not over-engineer types if they are getting to complex to used and the intention is not clear in the first place.

This would have prevented me from getting attacked on a personal level.


Please also think about PRs as a Pull Request which is indirectly always a proposal and needs to be accepted by the team/maintainers of a project, but can also get rejected / not merged.

@ST-DDT was telling me the last weeks that I should not open issues for something when I directly can open a PR, because (and I agree with that) in a PR we can discuss on a per code-line level.
In addition to that it prevents managing a flood of more and more issues.


I checked the template type of the TS definition Array/ReadonlyArray and it is indeed just and only T.
Also the interface Generator<T = unknown, TReturn = any, TNext = unknown> example is a good showcase for a real-world example.

So yes we should discuss two points:

  1. where is it okay to just use T, because I gave the argument above that TValue/TInput vs TResult makes it more clear to read the intent out of the name of the generic type without reading left or right from it. But in our cases we have these examples only on separate places and therefore not the case where we would have two generics next to each other.
  2. Should we follow the suggestion of @mattpocock and use the T prefix for template types in general, or should we not use it, but maybe come to a compromise where we use the non-prefixed but more expressive variant:
    - maybe<T>(callback: () => T, options: ...): T | undefined;
    + maybe<Result>(callback: () => Result, options: ...): Result | undefined;

@ST-DDT
Copy link
Member

ST-DDT commented Apr 15, 2023

I would like to remember @xDivisionByZerox and @ST-DDT to read our Code of Conduct again.

Thanks for pointing this out. I'll try to improve on this.
But I would also like to ask yourself to do the same regarding your prior rude comments.
Lets talk about this in person as to not further get off topic with the discussion.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 20, 2023

Team Decision

  • We want to name our generics T or TPrefixed.
  • We only use TPrefixed if this adds value to the declaration, decided on a case by case role
    • if there are multiple generic paramters

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed needs rebase There is a merge conflict s: needs decision Needs team/maintainer decision labels Apr 20, 2023
@ST-DDT ST-DDT marked this pull request as draft April 20, 2023 16:58
@Shinigami92 Shinigami92 requested review from ST-DDT, xDivisionByZerox and a team April 20, 2023 17:14
@Shinigami92 Shinigami92 marked this pull request as ready for review April 20, 2023 17:22
@Shinigami92
Copy link
Member Author

@faker-js/members @faker-js/maintainers Please write new review comments on each remaining rename with a reason why or why not it should be named T / not T, if it hurts you in your soul

scripts/generateLocales.ts Outdated Show resolved Hide resolved
src/modules/helpers/unique.ts Outdated Show resolved Hide resolved
src/modules/person/index.ts Outdated Show resolved Hide resolved
src/utils/types.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested a review from ST-DDT April 20, 2023 18:15
ST-DDT
ST-DDT previously approved these changes Apr 20, 2023
@ST-DDT ST-DDT requested a review from a team April 20, 2023 18:26
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label May 1, 2023
@ST-DDT
Copy link
Member

ST-DDT commented May 1, 2023

This has gotten stale over the last merges.

@Shinigami92 Shinigami92 requested a review from ST-DDT May 3, 2023 10:44
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label May 3, 2023
@ST-DDT ST-DDT merged commit 2e9ed96 into next May 4, 2023
17 checks passed
@ST-DDT ST-DDT deleted the rename-generics branch May 4, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior 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.

None yet

4 participants