Hi Jane, Hi Jack,
Thanks for sharing your thoughts. It makes sense. Did you mean literally
all classes in table-api-java, table-api-java-bridge, and table-common
modules should have at least one visibility annotations? Any other modules
or only these three modules?
Agree, we should add ArchUnit
Thanks for the updates. LGTM now.
Hi Jing,
In my opinion, enforcing visibility annotations for all classes in the API
module has a major benefit of simplifying CI checks for missing
annotations.
Best,
Jark
On Mon, 31 Jul 2023 at 12:33, liu ron wrote:
> Hi, Jane
>
> Thanks for driving this
Hi, Jane
Thanks for driving this discussion, I think this would be very useful for
developer, +1.
Best,
Ron
Jane Chan 于2023年7月31日周一 10:57写道：
> Hi Jing,
>
> According to your proposal, does it mean that there will be no public class
> > that has no annotation?
>
>
> Yes, every class should be
Hi Jing,
According to your proposal, does it mean that there will be no public class
> that has no annotation?
Yes, every class should be marked with the visibility annotation, but this
is restricted only to the classes in the table-api-java,
table-api-java-bridge, and table-common modules, as
Hi Jane,
Thanks for the clarification. Commonly speaking, it is a good idea to show
the clear information that a class is only used internally, i.e. please
don't rely on it. However, the rule of using @Internal in the community is
not clearly defined, at least it is not as clear as
Hi all,
Thanks for your valuable feedback. Here are my thoughts.
To Jark
I agree with your suggestions, and I've updated the sheet accordingly.
To Lincoln
> For the `DynamicFilteringEvent`, tend to keep it 'internal' since it's a
> concreate implementation of `SourceEvent`(and other two
Hi Jane,
Thanks for your effort of walking through all classes and compiling the
sheet. It is quite helpful. Just out of curiosity, do we really need to
mark some many classes as @Internal? What is the exactly different between
a public class with no annotation and with the @Internal?
Best
Hi Jane,
Thanks for driving this! Overall the proposed annotations looks good to me.
Some comments for the table[1]:
For the `DynamicFilteringEvent`, tend to keep it 'internal' since it's a
concreate implementation of `SourceEvent`(and other two implementers are
not public ones) .
For the
Hi Jane,
Thanks for kicking off this work and collecting the detailed list.
+1 to add the missing annotation.
This often confuses me whether the class can be modified without breaking
the compatibility
when looking at classes in table-common and table-api. Explicitly mark the
visibility can be
Hi, Devs,
I would like to start a discussion on adding missing visibility annotation
(PublicEvolving / Internal etc.) for Table APIs.
The motivation for starting this discussion was during the cleanup of which
Table API to be deprecated for version 2.0, I noticed that some of the APIs
lack
10 matches
Mail list logo
