Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15958 closed enhancement (fixed)

[patch][cla]remove cruft from charting

Reported by: ben hockey Owned by: cjolif
Priority: undecided Milestone: 1.9
Component: Charting Version: 1.8.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by ben hockey)

in quite a few of the dojox/charting files, there are unused variables and some unused AMD dependencies that can be removed. i'm planning to add patches to this ticket for these files a few at a time (as i come across them).

Attachments (7)

15958-axis2d.diff (2.3 KB) - added by ben hockey 7 years ago.
15958-action2d.diff (3.7 KB) - added by ben hockey 7 years ago.
15958-plot2d.diff (15.8 KB) - added by ben hockey 7 years ago.
15958-plot3d.diff (1.3 KB) - added by ben hockey 7 years ago.
15958-themes.diff (3.9 KB) - added by ben hockey 7 years ago.
15958-widget.diff (3.2 KB) - added by ben hockey 7 years ago.
15958-charting.diff (5.1 KB) - added by ben hockey 7 years ago.
diff for files in dojox/charting root dir

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by ben hockey

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

Changed 7 years ago by ben hockey

Attachment: 15958-axis2d.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-action2d.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-plot2d.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-plot3d.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-themes.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-widget.diff added

Changed 7 years ago by ben hockey

Attachment: 15958-charting.diff added

diff for files in dojox/charting root dir

comment:2 Changed 7 years ago by ben hockey

Description: modified (diff)

comment:3 Changed 7 years ago by ben hockey

Summary: remove cruft from charting[patch][cla]remove cruft from charting

i've attached all the files from a quick look at all of dojox/charting.

comment:4 Changed 7 years ago by cjolif

In [29667]:

refs #15958. Removing un-used code in action2d & axis2d packages. Thanks neonstalwart (CLA). !strict.

comment:5 Changed 7 years ago by cjolif

In [29668]:

refs #15958. Removing un-used code in themes & plot3d packages. Thanks neonstalwart (CLA). !strict.

comment:6 Changed 7 years ago by cjolif

I started looking at the patches and committed some. Thanks. Looking at the code I have some doubts about your modifications to plot2d/common.js. Looks like you you are now in some cases comparing new potential min/max with the old min/max value (before the iteration) while before it was compared with the "live" value (updated at each iteration). Am I missing something or have you done more than removing dead code here? (and created a potential issue).

Last edited 7 years ago by cjolif (previous) (diff)

comment:7 Changed 7 years ago by cjolif

Also in dojox/charting/widget/BidiSupport.js you are removing dependencies on ../BidiSupport but even if the returned value of the parent module is not explicitly used I'm pretty sure widget/BidiSupport won't work without ../BidiSupport.

comment:8 Changed 7 years ago by ben hockey

thanks for taking my patches.

sorry about the regressions, now i can see that i've changed the logic in plot2d/common.js. i was trying to micro-optimize by reducing property lookups. it should have been more like:

// inside the loop - use a var for the running value to avoid property lookups on stat
old_vmin = Math.min(old_vmin, y);

// after the loop - assign the var to the stats
stats.vmin = old_vmin;

but i'm not even sure at this point if that pattern is quite right so you can leave out these changes (unless you want to spend time fixing what i was trying to do, but i don't expect that you should). i'll likely be submitting more patches in the future and i might pick that up again at a later time.

as for BidiSupport?... i was probably overzealous with those. i think i just saw unused arguments and removed them - i know i would have tried to see if they had side-effects but i guess i missed what these were doing.

i have to say that without tests it was like i was flying blind. i'm used to leaning on tests to find these kinds of refactoring errors before i submit patches. i tried to make sure i didn't introduce any issues but i can see now that i missed some. you're likely to be more familiar with the code than me so i won't be offended if you adjust my patches.

at some point maybe there can be a concentrated effort on making tests for charting so that it's easier for others to contribute patches and feel confident that they haven't introduced regressions. looking at the code though, it might take some refactoring just to find a way to easily write tests for some of this code.

again, sorry for the regressions.

comment:9 Changed 7 years ago by cjolif

no pb Ben. I fully agree charting would require a lot of more regression testing to be put in place.

comment:10 Changed 7 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [29673]:

fixes #15958. Removing un-used code in plot2d, widget & base packages (all contributed patches but BidiSupport? & common.js & a few things that will be included in another pending patch to avoid patches to conflict). Thanks neonstalwart (CLA). !strict.

comment:11 Changed 7 years ago by cjolif

Milestone: tbd1.8.1

comment:12 Changed 7 years ago by cjolif

Milestone: 1.8.11.9
Note: See TracTickets for help on using tickets.