Skip to content

Javadoc for expressions #1059

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

Merged
merged 4 commits into from
Jan 27, 2023
Merged

Javadoc for expressions #1059

merged 4 commits into from
Jan 27, 2023

Conversation

katcharov
Copy link
Collaborator

Partial work for JAVA-4799

Covers boolean, date, number, integer, and expression, but not others, since PRs are still in progress.

There are naming changes, to parameters especially.

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I reviewed only Expression.

@katcharov katcharov requested review from jyemin and rozza December 6, 2022 17:25
* The method {@link Expression#eq} should be used to compare values for
* equality.
*/
@Deprecated // TODO-END (?) <p>Marked deprecated to prevent erroneous use.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never, ever seen this done. Have you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen abuses, though not the deprecation of equals in particular. I don't plan to argue for it or keep it, but I was worried that users will mistake it for eq, and was hoping for some creative way to address this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I removed this)

@@ -21,116 +21,202 @@
import java.util.function.Function;

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you plan to write here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think:

A value that may be stored in or otherwise operated upon by MongoDB.

Followed by some less-dense variation on:

This API is a language binding: it is a way to use a remote system as if
it were a native Java API. It is a way to represent values and computations
in some "execution context". This API is also a Domain Specific Language: it
is the Java-native variant of MQL, which is used to query MongoDB or (more
broadly) to work with data on MongoDB.

It would be followed by a few paragraphs covering important points and gotchas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a long description (similar to Stream). Unsure which parts of it, if any, might be moved to the package docs (none are obviously unrelated to the type hierarchy of which this is the root).

@@ -17,45 +17,43 @@
package com.mongodb.client.model.expressions;

/**
* Expresses a boolean value.
* A logical boolean value, either true or false.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we've been over this, but I'm still having difficulty squaring this sentence with the service that this API actually provides in the context of a MongoDB application. To my ears, this sentence implies that there should be a method like boolean booleanValue() (just like java.lang.Boolean has) on this type, which of course makes no sense. If you had to write another paragraph expanding on what this means, what would it say?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katcharov katcharov requested a review from stIncMale January 12, 2023 18:46
@katcharov katcharov requested a review from stIncMale January 17, 2023 19:56
@katcharov katcharov marked this pull request as ready for review January 17, 2023 23:36
@katcharov katcharov force-pushed the expressions-context branch from f6e5a14 to 1fb5893 Compare January 18, 2023 17:35
@katcharov
Copy link
Collaborator Author

Rebased. Conflicts on switch/pass (none on MqlEx, which should confirm that the correct signature was used).

*
*
* <p>Users should treat these interfaces as sealed, and should not create
* implementations.
*
* @see Expressions
*/
@Evolving
public interface Expression {
Copy link
Member

@stIncMale stIncMale Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure that the following unchecked methods are documented as such (I am using the annotation from #1068 to express what the user asserts when using any of these methods):

method annotation
Branches.isArray @MqlUnchecked(TYPE_ARGUMENT)
Branches.isMap @MqlUnchecked(TYPE_ARGUMENT)
BranchesIntermediary.isArray @MqlUnchecked(TYPE_ARGUMENT)
BranchesIntermediary.isMap @MqlUnchecked(TYPE_ARGUMENT)
Expressions.currentAsMap @MqlUnchecked(TYPE_ARGUMENT)
Expressions.ofMap(Bson) @MqlUnchecked(TYPE_ARGUMENT)
Expression.isArrayOr @MqlUnchecked(TYPE_ARGUMENT)
Expression.isMapOr @MqlUnchecked(TYPE_ARGUMENT)
ArrayExpression.elementAt @MqlUnchecked(PRESENT)
ArrayExpression.first @MqlUnchecked(PRESENT)
ArrayExpression.last @MqlUnchecked(PRESENT)
MapExpression.get(key) @MqlUnchecked(PRESENT)
DocumentExpression.getField @MqlUnchecked(PRESENT)
DocumentExpression.getBoolean(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getNumber(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getInteger(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getString(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getDate(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getDocument(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getMap(fieldName) @MqlUnchecked({PRESENT, TYPE})
DocumentExpression.getArray(fieldName) @MqlUnchecked({PRESENT, TYPE})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will apply this in the final doc PR which covers docs etc. (along with, for example, server version annotations).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the annotation may be added later. This comment is about documenting all unchecked methods with text (unless you decide not to do so and use only the annotation instead, which is what I would have preferred). The reason I used the annotation in the table is to conveniently express what a user must assert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave unresolved, for easier reference.

@rozza rozza removed their request for review January 19, 2023 14:28
@katcharov katcharov force-pushed the expressions-context branch from 1fb5893 to 458d786 Compare January 19, 2023 15:48
Base automatically changed from expressions-context to expressions January 19, 2023 15:48
@katcharov katcharov requested a review from stIncMale January 20, 2023 16:28
Comment on lines +24 to +34
/**
* Documents places where the API relies on a user asserting
* something that is not checked at run-time.
* If the assertion turns out to be false, the API behavior is unspecified.
*
* <p>This class is not part of the public API and may be removed or changed at any time</p>
*/
@Documented
@Retention(RetentionPolicy.SOURCE)
@Target({ElementType.METHOD, ElementType.TYPE_USE})
public @interface MqlUnchecked {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note how this annotation is documented as not being a part of the public API despite residing in a non-internal package and being public. This is because we want to generate the API docs for this annotation, but we don't want users to use it either to annotate their code, or to analyze the code at compile time.

@katcharov katcharov requested review from rozza and removed request for jyemin January 23, 2023 15:58
@katcharov katcharov merged commit 8978839 into expressions Jan 27, 2023
@katcharov katcharov deleted the expressions-docs1 branch January 27, 2023 23:49
katcharov added a commit that referenced this pull request Jan 30, 2023
katcharov added a commit that referenced this pull request Jan 31, 2023
katcharov added a commit that referenced this pull request Jan 31, 2023
* Implement boolean expressions (#1025)

JAVA-4779

* Implement filter, map, reduce (#1031)

JAVA-4781

* Implement eq, ne, gt, gte, lt, lte (#1033)

JAVA-4784

* Implement string expressions (#1036)

JAVA-4801

* Implement arithmetic expressions (#1037)

Implement arithmetic expressions (from top 50, and others)

JAVA-4803

* Implement array expressions (#1043)

JAVA-4805

* Implement date expressions (#1045)

JAVA-4804

* Implement conversion/type expressions (#1050)

JAVA-4802

* Implement document expressions (#1052)

JAVA-4782

* Replace reduce with individual reductions (#1053)

JAVA-4814

* Implement map expressions (#1054)

JAVA-4817

* Implement switch expression (#1055)

JAVA-4813

* Test expressions in context (#1057)

JAVA-4820

* Add javadoc for boolean, date, number, integer, and expression (#1059)

 JAVA-4799

* Update and add documentation (#1059)

* Fix, tests

 JAVA-4799

* Add `@MqlUnchecked` and a few usage examples (#1059)

 JAVA-4799

* Add has to document, add tests (#1070)

 JAVA-4799

* Add javadocs for remaining classes (#1070)

 JAVA-4799

* 5.2 annotations (#1070)

 JAVA-4799

* 5.0 annotations (#1070)

 JAVA-4799

* 4.4 annotations (#1070)

 JAVA-4799

* 4.2 annotations (#1070)

 JAVA-4799

* 4.0 annotations (#1070)

 JAVA-4799

* Update and add documentation, add tests, fix minor issues (#1070)

Rename extractBsonValue

Fix access modifiers

Remove excess comments

Update docs

Fix: behaviour of get

Add notNull to API, add notNullApi test

Fix docs/annotations, tests

Fix docs, annotations, since

Fix docs

Revert external

Add missing MqlUnchecked

Fix missing null checks

Checkstyle

JAVA-4799

* Rename to Mql (automated) (#1073)

JAVA-3879

* Rename methods (automated) (#1073)

JAVA-3879

* Update naming, terms, and missing checks and annotations (#1073)

JAVA-3879

---------

Co-authored-by: Valentin Kovalenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants