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(TS) remove enums for string unions when enum is not necessary #8918

Merged
merged 3 commits into from
May 14, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented May 14, 2023

Motivation

As per discussion, our use of enums isn't justified.
#8915 (comment)

Enums aren't supposed to be named dictionary of strings, we have union types for that.

There was also a possible bug in the glPrecision code in which we were transforming an object into an array, without knowing in which order we were loopin through precisions test

@asturur
Copy link
Member Author

asturur commented May 14, 2023

We could have a second PR in which we explore if common strings used over and over should be a constant.

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2023

Build Stats

file / KB (diff) bundled minified
fabric 900.521 (-1.895) 303.961 (-0.625)

@asturur
Copy link
Member Author

asturur commented May 14, 2023

This PR is also probably fixing most of your extra code in the utils types @ShaMan123

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2023

Coverage after merging remove-enum-for-unions into master will be

83.64%

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%101, 104, 207–208, 233–234
   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
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/Pattern
   Pattern.ts92.21%91.89%90%93.33%116, 127, 136, 29, 92
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–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%106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
   Canvas.ts78.87%77.54%81.67%79.41%1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
   SelectableCanvas.ts94.39%91.16%94.55%96.63%1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
   StaticCanvas.ts96.85%92.86%100%98.61%1113–1114, 1114, 1114–1115, 1235, 1245, 1299–1300, 1303, 1338–1339, 1416, 1425, 1425, 1429, 1429, 1476–1477, 321–322, 339, 770, 782–783
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%314–315, 319–320, 323–324, 42, 72–73, 73, 75, 75, 75–76, 78–79
   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%232, 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.ts100%100%100%100%
   node.ts73.08%33.33%66.67%88.24%27, 31–32, 32, 32, 43
src/filters
   BaseFilter.ts22.04%23.21%32%19.05%100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157, 171–176, 180–181, 181, 181–184,

@asturur
Copy link
Member Author

asturur commented May 14, 2023

i m moving forward with this, i don't see possible disagreemts since it spawned from your question.
In case i m fine reverting and re-opening.

@asturur asturur merged commit d419ff1 into master May 14, 2023
18 checks passed
@asturur asturur deleted the remove-enum-for-unions branch May 14, 2023 08:32
@@ -1,8 +1,4 @@
export enum GLPrecision {
Copy link
Contributor

@ShaMan123 ShaMan123 May 14, 2023

Choose a reason for hiding this comment

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

This enum I liked
But it is ok to remove
I would add the p suffix in our code just to it make it more human and readable...
But I am sure there are legit reasons why not.

@asturur asturur mentioned this pull request May 19, 2023
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.

None yet

2 participants