Fix Private_Access_Error in Ordered_Multi_Value_Key
#14543
Conversation
GregoryTravis
requested review from
AdRiley,
hubertp and
jdunkerley
as code owners
GregoryTravis
added
the
CI: No changelog needed
GregoryTravis
changed the title
rivate_Access_Error in Ordered_Multi_Value_KeyPrivate_Access_Error in Ordered_Multi_Value_Key
JaroslavTulach
left a comment
JaroslavTulach
left a comment
There was a problem hiding this comment.
Please hide (if it is not possible to remove) test only APIs.
| ## --- | ||
| private: true | ||
| --- | ||
| java_column_test_only self = self.java_column |
There was a problem hiding this comment.
- Can this test only method be move outside of API exposed by
from Standard.Table import all, please?
There was a problem hiding this comment.
How can that be done without a non-private method in this file?
There was a problem hiding this comment.
- The method can be in any other file/module in this project
- Especially suitable are files/module not re-exported from
Main.enso - then, when one types
from Standard.Table import allone doesn't get such file/module and itstest_only_java_columnmethod in the "all" imports- some call such a not-all API a veil API
- there is for example
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/In_Memory_Visualization_Helpers.enso... - having
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/In_Memory_Test_Only_Helpers.ensowould be a nice choice from my point of view as well - btw. this line of thought is opposite to the other one - should this be implemented the other one is void
There was a problem hiding this comment.
Thank you, I didn't realize an internal module could be used for this.
| compare_ordered_keys make_key table compare_keys | ||
|
|
||
| run_java table = | ||
| java_columns = table.columns.map column-> (column:In_Memory_Column).java_column_test_only |
There was a problem hiding this comment.
- Why are benchmarks accessing internals of memory table?
- Shouldn't we benchmark end-user functionality and not internals?
There was a problem hiding this comment.
We should benchmark anything whose performance we want to track. There is no direct end-user use of the multi-value keys, but they should still be benchmarked. I don't think it makes sense to convert a benchmark to Java just to remove a hidden but non-private accessor.
There was a problem hiding this comment.
I don't see that we have any benchmarks of code in std-bits, but sometimes we do need it. This is that case.
There was a problem hiding this comment.
- Wouldn't it be easier to run the
test/Benchmarkswith--disable-private-checksthen? E.g.
sbt:enso> runEngineDistribution --run test/Benchmarks --disable-private-check
- that would be way better than to spoil the API consumed by Enso users just because of internal benchmarking in my architect's view of the affairs
- if so, then just expand Internal tests run with --disable-private-check #10743 to recognize
test/Benchmarksand modifystd-benchmarksrunner to disable private checks as well - btw. this line of thought is opposite to the other one - should this be implemented the other one is void
Added a test-only accessor.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.