Align radialLinear coding style

[benmccann]

Align radialLinear coding style with the rest of the files in the project by moving functions outside module.exports

[kurkle]

This change is +19 bytes to Chart.min.js
Moving functions in other scales inside module.exports would be -43 bytes
So is this really the way to go?

[benmccann]

Happy New Year @kurkle! Thanks for taking a look

Good point on the size. One thing I like about them being outside is that it's easier from a coding perspective to have one less level of indentation. It also makes it a little easier to see what's private vs in the public API when in the file. I'm also not sure how much we should prioritize micro optimizations and think our time might be better spent fixings bugs, etc. than doing before and after size comparisons on every change.

@simonbrunel started this convention. I'm mainly just trying to make sure we have one consistent convention that we follow everywhere and don't care as much about which way we pick. @simonbrunel thoughts on this one?

[simonbrunel]

@benmccann it's not a convention! I'm working on removing the function(Chart) in all files to make the project fully importable (wip #4478). I was waiting to get most scale related PRs merged in order to finalize this task to prevent merge conflicts. I already have a branch with these changes and could try to submit a PR soon, however it would conflict with many PR.

[benmccann]

Files are importable whether the functions are inside or outside the module.exports though, right? And either way this is a step towards what you're trying to accomplish. Though I can close it if it will conflict with your branch.

It might make sense to go ahead and submit your PR. The number of PRs here just keeps growing, so it seems like you'll only ever get further away. And I'm imagining the PR to make the scales importable generally won't conflict with most of the PRs since it will primarily touch the module.exports and require statements, which most PRs here don't touch.

If we want to get the scale-related PRs merged, perhaps we should ask the maintainers to focus their reviews on those and hold off on submitting new scale PRs until your PR makes its way in? I've listed the status of the scale PRs below

Pending:

• Fix generateTicks when working with tiny numbers #5948 (not impacted: changes outside the export)
• Return correct label for value type axis #5920
• Polar area: align start angle with angleLines #5919
• (re)allow modifying ticks in afterBuildTicks #5913 (not impacted: core/core.scale.js)

Stale:

• Float-bar support. The logic is to use array values [bottomY, topY] i… #5262
• Rotations between 180-360 degrees on x axis labels #5246 (not impacted: core/core.scale.js)
• Scales are measuring the longest text for all ticks including skipped… #5765 (not impacted: core/core.scale.js)

Merged:

• Fix time scale ticks.reverse issue #5933

Closed:

• Improve offset calculation for scale.offset option #4658

[simonbrunel]

And I'm imagining the PR to make the scales importable generally won't conflict with most of the PRs since it will primarily touch the module.exports and require statements, which most PRs here don't touch.

Not true, removing the function export also removes an indent level.

[benmccann]

Another option might be to make the scales importable one at a time. That might be a lot easier to coordinate than getting all nine of those PRs merged and freezing work on all other scale PRs. E.g. this PR could be merged now which would affect very little. And there's a number of other changes we could make that would require only a single PR to be merged in order to unblock making that particular scale importable

[simonbrunel]

I'm going to rebase my branch and submit a PR because from the pending PR, only 2 are concerned, #5920 is almost merged and #5919 will be straightforward to rebase. #5262 is outdated, previous rebase reverted some code so in all cases it will need a deep rework.

[benmccann]

Closing in favor of #5953