-
Notifications
You must be signed in to change notification settings - Fork 15
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
Refactor an internal API system (part 1: new class system & remove all internal properties from DSL) #376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the first part regarding the API
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/accessors.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/accessors.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandler.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/BindingHandlerDefault.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/DataFramePlotBuilder.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupByPlotBuilder.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupByScope.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupedDataScope.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/LayerBuilderImpl.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/CustomPlotBuilder.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupedDataScope.kt
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/GroupedDataScope.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/SingleLayerPlotBuilder.kt
Outdated
Show resolved
Hide resolved
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/CoordFlip.kt
Show resolved
Hide resolved
...t/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/ConstantSetter.kt
Outdated
Show resolved
Hide resolved
...ot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithIntercept.kt
Outdated
Show resolved
Hide resolved
...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithLower.kt
Outdated
Show resolved
Hide resolved
...-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithMiddle.kt
Outdated
Show resolved
Hide resolved
...ts-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithSize.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
@devcrocod add |
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/Layout.kt
Outdated
Show resolved
Hide resolved
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/Layout.kt
Outdated
Show resolved
Hide resolved
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/feature/Layout.kt
Outdated
Show resolved
Hide resolved
kandy-api/src/main/kotlin/org/jetbrains/kotlinx/kandy/dsl/internal/validation.kt
Outdated
Show resolved
Hide resolved
...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithSlope.kt
Outdated
Show resolved
Hide resolved
...ts-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/aes/WithXEnd.kt
Outdated
Show resolved
Hide resolved
...s-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/layers/builders/ABLineBuilder.kt
Outdated
Show resolved
Hide resolved
include("kandy-util") | ||
|
||
include("examples:idea-examples:lets-plot-simple") | ||
include("examples:idea-examples:echarts-simple") | ||
//include("examples:idea-examples:echarts-simple") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be fixed in the 4th PR.
* - [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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ x]- > [x]?
* - [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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ x] -> [x]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and all [ x] inclusions through the whole project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KDocs works incorrect here without space for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also this PR is not about echarts documentation at all, let's discuss it in future.
class LayerCreatorScopeTest { | ||
|
||
@Test | ||
fun `test addLayer`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need here a test with addition of a few layers in the correct order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests are fixed in the next PR.
import org.jetbrains.kotlinx.kandy.ir.bindings.PositionalMapping | ||
|
||
internal fun <T> PlotContext.x( | ||
internal fun <T> MultiLayerPlotBuilder.x( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rename xy.kt to something else? MultiLayerPlotBuilder
XY in naming?
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest avoiding using value
for variable names and parameter names, it's too common and leads to nothing, could you please rename it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's literally "value". What alternative do you suggest? settingValue
?
*/ | ||
open fun <DomainType, RangeType> addNonPositionalMapping( | ||
aes: Aes, | ||
values: List<DomainType>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domainTypes instead of values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? You provide values of DomainType
here, not types.
*/ | ||
public abstract class CustomPlotBuilder: PlotBuilder { | ||
/** | ||
* Plot builder dataset handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to use one-row Kdocs
/** Plot builder features collector. */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but I prefer "expanded" ones.
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
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 unchangedFixes #163, fixes #378, fixes #379, fixes #380, fixes #381.