Skip to content
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

Some updates to docs #1534

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Some updates to docs #1534

merged 6 commits into from
Dec 1, 2023

Conversation

erlendvollset
Copy link
Collaborator

@erlendvollset erlendvollset commented Nov 30, 2023

  • Improve filter documentation
  • Revise quickstart
  • Make TOC slightly more granular. time series and geospatial each have 4 sub resources, and sequences has 2, so bringing them up makes more sense IMO.

(TY ChatGPT) 🙏

@erlendvollset erlendvollset requested review from a team as code owners November 30, 2023 17:29
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #1534 (d0edd32) into master (e2624e6) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1534   +/-   ##
=======================================
  Coverage   91.83%   91.83%           
=======================================
  Files         120      120           
  Lines       15591    15591           
=======================================
  Hits        14318    14318           
  Misses       1273     1273           
Files Coverage Δ
cognite/client/data_classes/filters.py 98.07% <ø> (ø)

Feature data modeling instead of assets, as this is the functionality we want new users to relate to.
@erlendvollset erlendvollset changed the title Improve filter docs Some updates to docs Nov 30, 2023
Time Series and geospatial have 4 sub resources, sequences has 2, so it makes more sense to group them like this
Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

I really like that you have made a separate top-level page in the docs for filtering. Should allow us to link to it easily from other parts of the documentation without bloating it with (similar-in-nature) examples everywhere 🥇

_filter_name = "or"


@final
class Not(CompoundFilter):
"""A filter that negates another filter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""A filter that negates another filter.
"""A filter that negates another filter recursively.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about that. It makes it sound like it negates each of the underlying filters recursively, which would be difficult to reason about if it was the case.

Comment on lines +407 to +410
gt (FilterValue | None): Greater than.
gte (FilterValue | None): Greater than or equal to.
lt (FilterValue | None): Less than.
lte (FilterValue | None): Less than or equal to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that these type hints using FilterValue are wrong/misleading as only a subset is supported (other misleading uses: Overlaps and Prefix):

(RangeValue (RangeValue (string) or RangeValue (number) or RangeValue (integer))) or ReferencedPropertyValueV3 (object) (FilterValueRange)

@dataclass
class PropertyReferenceValue:
    property: PropertyReference

@dataclass
class ParameterValue:
    parameter: str

FilterValue: TypeAlias = Union[RawValue, PropertyReferenceValue, ParameterValue]
RawValue: TypeAlias = Union[str, float, bool, Sequence, Mapping[str, Any], Label]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, not ideal. Will leave some validation for runtime. But not gonna address that in this PR.

cognite/client/data_classes/filters.py Outdated Show resolved Hide resolved
cognite/client/data_classes/filters.py Show resolved Hide resolved
docs/source/filters.rst Outdated Show resolved Hide resolved
docs/source/filters.rst Outdated Show resolved Hide resolved
docs/source/filters.rst Outdated Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@erlendvollset erlendvollset merged commit 6ec2c15 into master Dec 1, 2023
8 checks passed
@erlendvollset erlendvollset deleted the fix-filter-docs branch December 1, 2023 11:38
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.

2 participants