Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376

AndreiKingsley · 2024-04-25T13:59:38Z

This is the first part of several PRs with internal API redesigns. In this one, I reworked the class system used to build DSLs. The PR is big, but the minimum possible because it changes the root of the system.

What went into this PR

A new system of DSL interfaces and classes, most classes/properties are now internal
Implementations of these classes for Lets-Plot
You can build api and lets-plot modules (and test them in a Kotlin Notebook, for example).

What is NOT included in this PR:

DatasetHandler is still unchanged
echarts module (only some rename was done)
Tests
- I partially changed the tests in the api module to make them consistent with the new API, but a few did not make it yet - I commented them out; the tests in the api module are fully buildable and there're passed
- In lets-plot the tests are not consistent
  - Statistics needs to be updated as well
Documentation

Fixes #163, fixes #378, fixes #379, fixes #380, fixes #381.

devcrocod

I looked at the first part regarding the API

settings.gradle.kts

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/accessors.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandler.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandlerDefault.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/DataFramePlotBuilder.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupByPlotBuilder.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupByScope.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupedDataScope.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/LayerBuilderImpl.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/CustomPlotBuilder.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupedDataScope.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/SingleLayerPlotBuilder.kt

kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/CoordFlip.kt

...t/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/ConstantSetter.kt

...ot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithIntercept.kt

...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithLower.kt

...-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithMiddle.kt

...ts-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithSize.kt

devcrocod

It is necessary to write KDoc and check the current ones. Also, in abstract classes, you can use protected instead of internal, so that properties will be accessible in subclasses.

AndreiKingsley · 2024-05-29T11:04:24Z

In most cases, the properties need to be accessible outside of the class so they are made internal (i.e. actually public, but for modules, not to the user - what I called semi-internal)

AndreiKingsley · 2024-05-29T11:13:37Z

@devcrocod add ColumnsContainer to GroupedDataScope with last commit, check please

kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/Layout.kt

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/validation.kt

...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithSlope.kt

...ts-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithXEnd.kt

...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/ABLineBuilder.kt

zaleslaw · 2024-06-10T11:02:41Z

settings.gradle.kts

 include("kandy-util")

 include("examples:idea-examples:lets-plot-simple")
-include("examples:idea-examples:echarts-simple")
+//include("examples:idea-examples:echarts-simple")


Does it make sense to stay with commented subprojects? or better to remove it, or it removed only in this PR for the debug purposes?

Will be fixed in the 4th PR.

zaleslaw · 2024-06-10T11:03:30Z

kandy-echarts/src/main/kotlin/org/jetbrains/kotlinx/kandy/echarts/layers/layers.kt

- *  - [y][AreaContext.y] - mapping data on the y-axis.
- *  - [color][AreaContext.color] - area fill [color][org.jetbrains.kotlinx.kandy.util.color.Color].
- *  - [position][AreaContext.position] -
+ *  - [ x][AreaHandler.x] - mapping data on the x-axis.


[ x]- > [x]?

zaleslaw · 2024-06-10T11:03:56Z

kandy-echarts/src/main/kotlin/org/jetbrains/kotlinx/kandy/echarts/layers/layers.kt

- * - [y][BarContext.y] - mapping data on the y-axis.
- * - [color][BarContext.color] - bars [color][org.jetbrains.kotlinx.kandy.util.color.Color].
- * - [alpha][BarContext.alpha] - bars opacity.
+ * - [ x][BarHandler.x] - mapping data on the x-axis.


[ x] -> [x]?

and all [ x] inclusions through the whole project

KDocs works incorrect here without space for some reason.

And also this PR is not about echarts documentation at all, let's discuss it in future.

zaleslaw · 2024-06-10T11:08:00Z

kandy-api/src/test/kotlin/org/jetbrains/kotlinx/kandy/builders/layerCollectorContext.kt

+class LayerCreatorScopeTest {
+
+    @Test
+    fun `test addLayer`() {


Do we need here a test with addition of a few layers in the correct order?

All tests are fixed in the next PR.

zaleslaw · 2024-06-10T11:10:06Z

kandy-api/src/test/kotlin/org/jetbrains/kotlinx/kandy/dsl/impl/xy.kt

 import org.jetbrains.kotlinx.kandy.ir.bindings.PositionalMapping

-internal fun <T> PlotContext.x(
+internal fun <T> MultiLayerPlotBuilder.x(


Could we rename xy.kt to something else? MultiLayerPlotBuilder XY in naming?

zaleslaw · 2024-06-10T11:11:43Z

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandler.kt

+     * Adds a [positional setting][PositionalSetting] with the given aes and value.
+     *
+     * @param aes the aesthetic attribute (aes) to associate with the positional setting.
+     * @param value the value of the setting.


I suggest avoiding using value for variable names and parameter names, it's too common and leads to nothing, could you please rename it

Well, it's literally "value". What alternative do you suggest? settingValue?

zaleslaw · 2024-06-10T11:12:39Z

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandler.kt

+     */
+    open fun <DomainType, RangeType> addNonPositionalMapping(
+        aes: Aes,
+        values: List<DomainType>,


domainTypes instead of values?

Why? You provide values of DomainType here, not types.

zaleslaw · 2024-06-10T11:13:22Z

kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/CustomPlotBuilder.kt

+ */
+public abstract class CustomPlotBuilder: PlotBuilder {
+    /**
+     * Plot builder dataset handler.


Feel free to use one-row Kdocs

/** Plot builder features collector. */

Ok, but I prefer "expanded" ones.

refactor an internal API system

e8c438f

AndreiKingsley requested review from zaleslaw and devcrocod April 25, 2024 13:59

AndreiKingsley added 14 commits April 29, 2024 13:45

fix visibility and addDataset

a22607b

rename bindingHandler

7fce22d

move dataset index to constructor

7ba5034

move dataset index to constructor

6770b8c

grouped data groupBy for statistics

7e33cdb

grouped data fix

ad29f24

grouped data comment

f652153

comments

226955d

rename layer interfaces

10296c3

single layer plot

3c5f12b

custom plot builder

649744f

custom plot builder & grouped data fix

4d13cc3

grouped datasetHandler accessor

501a240

fix layout

379eb0d

AndreiKingsley added this to the Kandy release v0.7.0 milestone May 1, 2024

AndreiKingsley added 3 commits May 2, 2024 13:54

xy accessors fix

da23ab7

fix custom plot builder

864d4e7

fix version

56ec619

devcrocod requested changes May 27, 2024

View reviewed changes

fix binding handler and remove context parameters todo in docs

1e224bd

AndreiKingsley requested a review from devcrocod May 29, 2024 07:54

devcrocod reviewed May 29, 2024

View reviewed changes

modify grouped data scope

676d6fe

add docs

4e80721

AndreiKingsley requested a review from devcrocod May 30, 2024 10:31

fix fully qualified names

beba68e

devcrocod reviewed Jun 5, 2024

View reviewed changes

AndreiKingsley added 2 commits June 5, 2024 16:21

code fixes

5462cdd

fix modifiers for layer builders

fbcf40a

AndreiKingsley requested a review from devcrocod June 5, 2024 12:25

optimize imports

dc5e883

zaleslaw requested changes Jun 10, 2024

View reviewed changes

rename xy

c7af6d7

AndreiKingsley requested a review from zaleslaw June 10, 2024 13:12

devcrocod approved these changes Jun 18, 2024

View reviewed changes

zaleslaw approved these changes Jun 18, 2024

View reviewed changes

AndreiKingsley closed this Jun 19, 2024

AndreiKingsley reopened this Jun 19, 2024

AndreiKingsley merged commit d578e30 into main Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376

Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376

AndreiKingsley commented Apr 25, 2024 •

edited

Loading

devcrocod left a comment

devcrocod left a comment

AndreiKingsley commented May 29, 2024

AndreiKingsley commented May 29, 2024

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024

zaleslaw Jun 10, 2024

zaleslaw Jun 10, 2024

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024

AndreiKingsley Jun 10, 2024

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024

zaleslaw Jun 10, 2024

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024 •

edited

Loading

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024

zaleslaw Jun 10, 2024

AndreiKingsley Jun 10, 2024 •

edited

Loading

Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376

Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376

Conversation

AndreiKingsley commented Apr 25, 2024 • edited Loading

devcrocod left a comment

Choose a reason for hiding this comment

devcrocod left a comment

Choose a reason for hiding this comment

AndreiKingsley commented May 29, 2024

AndreiKingsley commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiKingsley Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiKingsley Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

AndreiKingsley commented Apr 25, 2024 •

edited

Loading

AndreiKingsley Jun 10, 2024 •

edited

Loading

AndreiKingsley Jun 10, 2024 •

edited

Loading