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

rename(): IPoint => XY #8806

Merged
merged 3 commits into from
Mar 27, 2023
Merged

rename(): IPoint => XY #8806

merged 3 commits into from
Mar 27, 2023

Conversation

ShaMan123
Copy link
Contributor

Motivation

IPoint is a bad name

Description

A non brainer rename, nothing else

Changes

Gist

Thankfully VSCODE is a reliable tool

In Action

thankfully VSCODE is a reliable tool
@ShaMan123 ShaMan123 requested a review from asturur March 25, 2023 07:16
@github-actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 895.312 (-0.082) 304.351 (0)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2023

Coverage after merging IPoint-XY into master will be

83.71%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%100, 103, 206–207, 232–233
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Pattern.ts92.21%91.89%90%93.33%114, 138, 149, 158, 51
   Point.ts100%100%100%100%
   Shadow.ts98.51%96%100%100%168
   cache.ts97.06%90%100%100%56
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
   Canvas.ts78.98%77.54%81.67%79.60%1000, 1000, 1000–1002, 1004–1005, 1005, 1005, 1007, 1015, 1015, 1015–1017, 1017, 1017, 1023–1024, 1032–1033, 1033, 1033–1034, 1039, 1041, 1072–1074, 1077–1078, 1082–1083, 1197–1199, 1202–1203, 1276, 1396, 1519, 1589, 161, 186, 295–296, 299–303, 308, 331–332, 337–342, 36, 362, 362, 362–363, 363, 363–364, 372, 377–378, 378, 378–379, 381, 390, 396–397, 397, 397, 40, 440, 448, 452, 452, 452–453, 455, 537–538, 538, 538–539, 545, 545, 545–547, 567, 569, 569, 569–570, 570, 570, 573, 573, 573–574, 577, 586–587, 589–590, 592, 592–593, 595–596, 608–609, 609, 609–610, 612–616, 622, 629, 669, 669, 669, 671, 673–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 740, 740, 798–799, 799, 799–800, 802, 805–806, 806, 806–807, 809–810, 813, 813–815, 818–819, 889, 901, 908, 929, 961, 982–983, 999
   SelectableCanvas.ts94.39%91.16%94.55%96.63%1127, 1127–1128, 1131, 1151, 1151, 1209, 1262–1263, 1284, 1292, 1417, 1419, 1421–1422, 526, 706–707, 709–710, 710, 710, 759–760, 821–822, 875–877, 909, 914–915, 942–943
   StaticCanvas.ts96.01%91.48%100%97.93%1112–1113, 1113, 1113–1114, 1234, 1244, 1298–1299, 1302, 1337–1338, 1415, 1424, 1424, 1428, 1428, 1475–1476, 1690–1691, 311–312, 329, 409–410, 412–413, 774, 786–787, 872
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   polyControl.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts91.67%50%100%100%20
   node.ts76.92%50%66.67%88.24%27, 31–32, 32, 32
src/filters
   BaseFilter.ts21.93%22.81%32%19.05%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182,

@asturur
Copy link
Member

asturur commented Mar 27, 2023

I have still to understand why iPoint is bad, interface for point.

@asturur
Copy link
Member

asturur commented Mar 27, 2023

at some point XY will become IXY if we keep consistency where I are interfaces and T are types.

If you have 5 minutes, make an effort and explain why IPoint is bad.

@asturur asturur merged commit b218a0d into master Mar 27, 2023
@asturur asturur deleted the IPoint-XY branch March 27, 2023 11:01
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 28, 2023

I think that as a dev using fabric it is unnscessary overhead to have types starting with T and interface starting with I when all I want is to find the type easily and intuitively by guessing the name and autocomplete. I don't know if it is an interface or type and I don't want to know because I don't care. It is annoying to find it like that and opens up discussions like "should this type be an interface or a type?", which is a waste of time and effort. We just want to get it done.
IPoint is a name from the definately typed package, historical remains.
XY is a good name, self explanatory. All names should be like that.
As I mentioned previously, I don't think prefixing with T is a good idea as well for the reasons I just stated. But if we must lets stick to a single prefix for all types.

@ShaMan123
Copy link
Contributor Author

I think that as a dev using fabric it is unnscessary overhead to have types starting with T and interface starting with I when all I want is to find the type easily and intuitively by guessing the name and autocomplete. I don't know if it is an interface or type and I don't want to know because I don't care. It is annoying to find it like that and opens up discussions like "should this type be an interface or a type?", which is a waste of time and effort. We just want to get it done.
IPoint is a name from the definately typed package, hitorical remains.
XY is a good name, self explanatory. All names should be like that.
As I mentioned previously, I don't think prefixing with T is a good idea as well for the reasons I just stated. But if we must lets stick to a single prefix for all types.

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 28, 2023

But TXY is silly and not self explanatory
IXY the same

@asturur
Copy link
Member

asturur commented Apr 1, 2023

So your answer is not that IPoint was a bad name, but that you don't like the prefixing with I for interfaces and so since Point was already taken, for the class itself, XY is an alternative.

Regarding this:

I think that as a dev using fabric it is unnscessary overhead to have types starting with T and interface starting with I when all I want is to find the type easily and intuitively by guessing the name and autocomplete. I don't know if it is an interface or type and I don't want to know because I don't care. It is annoying to find it like that and opens up discussions like "should this type be an interface or a type?", which is a waste of time and effort. We just want to get it done.

Guessing the name and autocomplete is maybe your way to finding things that is fine. pressing T and knowing that most of the auto complete will be a type is another good way to do things.
The fact that you don't care if is an interfact or a type it means that either TS is badly done or that this is a personal preference.

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Apr 2, 2023

True, I don't understand the difference between type and interface. I do understand that interface defines an API and is meant to be used with the implements directive but I think type can be used for that as well.

Pressing T is a way to find types, absolutely. That is why I am against I. I don't think it is a good idea to press T and then press I. I won't be sure I didn't miss what I am looking for if there are 2 places to lookup instead of 1. Or if the type exists or not.
Another option is to export types as a named export. I prefer that. Then we don't need a prefix, we import types from fabric.
What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants