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

devcrocod · 2024-05-29T10:03:47Z

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

+// must be LayerCreatorScope, marker of containing grouped data
+// (needed for transformations, especially statistics)
+public sealed interface GroupedDataScope<T, R> {
+    public val key: ColumnGroup<T>


Still don't like using the key( This is not obvious to many, including me

It's not an obligation, but a convenience. Sometimes it's important to perform mapping from a key, so if you write a key.column, you won't make a mistake.

You can still write the same thing without the key
.

In this case I get an exception of different column sizes

This could be the case with stats because they don't currently provide context for the new dataframe - after these changes they will.

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

devcrocod · 2024-05-29T10:15:16Z

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

This class and MultilayerPlotBuilder raised questions for me. Why SingleLayerPlotBuider is inherited from PlotBuilder?

PlotBuilder has plot features regardless of whether the builder is a multi-layer, single-layer or any other type (for example build with series or any new way). For example - layout.

devcrocod · 2024-05-29T10:18:42Z

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

@@ -28,7 +29,8 @@ import org.jetbrains.kotlinx.kandy.ir.feature.PlotFeature
 * }
 * ```
 */
-public fun PlotContext.coordFlip() {
+public fun PlotBuilder.coordFlip() {


how many plot features do we have?

potentially as many as we want (in fact as lets-plot has)

It seems to me that all our features are mainly layer features, and we only have experimental features for plot

And what's problem with plot features?

Not a problem, just a fact: the map of features is there, but the features are not

...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

devcrocod · 2024-06-05T11:11:55Z

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

@@ -43,7 +46,8 @@ public inline fun PlotContext.layout(block: Layout.() -> Unit) {
 * Accessor for the [Layout] of a plot.
 * Returns an existing [Layout] if one exists, or creates a new one otherwise.
 */
-public val PlotContext.layout: Layout
+@Suppress("INVISIBLE_MEMBER")


Do you think that if we have several suppresses, maybe it’s better to apply them to the entire file?

devcrocod · 2024-06-05T11:12:53Z

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

@@ -4,12 +4,14 @@

 package org.jetbrains.kotlinx.kandy.letsplot.feature

-import org.jetbrains.kotlinx.kandy.dsl.internal.PlotContext
+import org.jetbrains.kotlinx.kandy.dsl.internal.MultiLayerPlotBuilder


unused import
set import optimization on commit

devcrocod · 2024-06-05T11:14:50Z

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

 import org.jetbrains.kotlinx.kandy.ir.feature.FeatureName
 import org.jetbrains.kotlinx.kandy.ir.feature.PlotFeature
 import org.jetbrains.kotlinx.kandy.letsplot.style.CustomStyle
 import org.jetbrains.kotlinx.kandy.letsplot.style.Theme
 import org.jetbrains.kotlinx.kandy.letsplot.style.Style
+import org.jetbrains.kotlinx.kandy.dsl.internal.*


set settings for specific import instead *

But then there would be a red highlighting

And I don't want to do auto-optimisation for the same reason

Hm, I tried this before and it didn't work for me back then, but now it does.
Well, then we need to add this suppressing to almost every file.

devcrocod · 2024-06-05T11:20:33Z

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

@@ -13,31 +13,9 @@ import org.jetbrains.kotlinx.kandy.ir.aes.Aes
 *
 * @throws IllegalArgumentException If the provided aesthetic value is not within the specified range.
 */
-public fun <T : Comparable<T>> checkInRange(aes: Aes, value: T, range: ClosedRange<T>): Unit =
+@PublishedApi
+internal fun <T : Comparable<T>> checkInRange(aes: Aes, value: T, range: ClosedRange<T>): Unit =


I think we can make an extension function here:

public fun <T: Comparable<T>> Aes.checkInRange(value: T, range: ClosedRange<T>): Unit

In this case, we can make it public, which will save us from having to suppress

devcrocod · 2024-06-05T11:44:44Z

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


    /**
     * Provides a constant setter for the `slope` value.
     *
     * @property slope A [ConstantSetter] object to directly set the slope.
     */
-    public val slope: ConstantSetter
-        get() = ConstantSetter(SLOPE, bindingCollector)
+    public val slope: org.jetbrains.kotlinx.kandy.letsplot.layers.builders.aes.ConstantSetter


remove qualifier name

devcrocod · 2024-06-05T11:45:33Z

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

-    public val xEnd: ConstantSetter
-        get() = ConstantSetter(X_END, bindingCollector)
+    public val xEnd: org.jetbrains.kotlinx.kandy.letsplot.layers.builders.aes.ConstantSetter
+        get() = org.jetbrains.kotlinx.kandy.letsplot.layers.builders.aes.ConstantSetter(


remove qualifier name

devcrocod · 2024-06-05T11:51:04Z

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

+    datasetIndex: Int = parent.datasetIndex
+) :
+    LayerBuilderImpl(parent, datasetIndex), ABLineBuilderInterface {
+    internal override val geom: Geom


You can make geom and requiredAes as protected in LayerBuilderImpl

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

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

devcrocod left a comment

devcrocod May 29, 2024

AndreiKingsley May 29, 2024

AndreiKingsley May 29, 2024 •

edited

devcrocod Jun 4, 2024

AndreiKingsley Jun 4, 2024

AndreiKingsley Jun 4, 2024

devcrocod May 29, 2024

AndreiKingsley May 29, 2024

devcrocod May 29, 2024

AndreiKingsley May 29, 2024 •

edited

devcrocod Jun 4, 2024

AndreiKingsley Jun 4, 2024

AndreiKingsley Jun 4, 2024

devcrocod Jun 5, 2024

devcrocod left a comment

AndreiKingsley commented May 29, 2024

AndreiKingsley commented May 29, 2024

devcrocod Jun 5, 2024

devcrocod Jun 5, 2024

devcrocod Jun 5, 2024

AndreiKingsley Jun 5, 2024

AndreiKingsley Jun 5, 2024

AndreiKingsley Jun 5, 2024

devcrocod Jun 5, 2024

AndreiKingsley Jun 5, 2024

devcrocod Jun 5, 2024

devcrocod Jun 5, 2024

devcrocod Jun 5, 2024

devcrocod Jun 5, 2024

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

Are you sure you want to change the base?

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

Conversation

AndreiKingsley commented Apr 25, 2024 • edited

devcrocod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiKingsley May 29, 2024 • edited

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 May 29, 2024 • edited

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

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

Choose a reason for hiding this comment

AndreiKingsley commented Apr 25, 2024 •

edited

AndreiKingsley May 29, 2024 •

edited

AndreiKingsley May 29, 2024 •

edited