Toniof 352 support typespecifiers #354

TonioF · 2020-11-02T17:12:27Z

Solves #352

forman

Very good and exhaustive work, thanks!

In addition to my comments and suggests ~~above~~below:

Please include a description of type specifiers and their usages into DataStore's class documentation. Refer to that doc from the methods that have a type_specifier argument are return type specifiers.
We also need to get right when to raise DataStoreError and when ValuError. Note, I often see that when you raise a ValueError, you include a type name in the message rather than the argument's name. Please make sure you include the argument names. ValueErrors are mainly developer errors.

forman · 2020-11-03T14:43:32Z

test/core/store/test_descriptor.py

        try:
            descriptor_dict = dict(data_id='xyz', type_id='tsr')
            DatasetDescriptor.from_dict(descriptor_dict)


Suggested change

try:

descriptor_dict = dict(data_id='xyz', type_id='tsr')

DatasetDescriptor.from_dict(descriptor_dict)

with self.assertRaises(ValueError) as cm:

descriptor_dict = dict(data_id='xyz', type_id='tsr')

DatasetDescriptor.from_dict(descriptor_dict)

self.assertEqual('...', f'{cm.exception}')

I can change that, but I'd like to point out that this is not part of the pull request.

forman · 2020-11-03T14:46:13Z

xcube/core/store/descriptor.py

-            raise ValueError(f'TypeId must be compatible with "geodataframe" type id, was {type_id}')
+    def _assert_type_specifier(self, type_specifier: str):
+        if not TYPE_SPECIFIER_GEODATAFRAME.is_compatible(type_specifier):
+            raise ValueError(f'TypeSpecifier must be compatible with "geodataframe" type specifier, '


Suggested change

raise ValueError(f'TypeSpecifier must be compatible with "geodataframe" type specifier, '

raise ValueError(f'type_specifier must be compatible with "geodataframe" type specifier, '

xcube/core/store/descriptor.py

forman · 2020-11-03T14:52:02Z

xcube/core/store/descriptor.py

-            raise ValueError(f'TypeId must be compatible with "mldataset" type id, was {type_id}')
+    def _assert_type_specifier(self, type_specifier: str):
+        if not TYPE_SPECIFIER_MULTILEVEL_DATASET.is_compatible(type_specifier):
+            raise ValueError(f'TypeSpecifier must be compatible with "mldataset" type specifier, was {type_specifier}')


Suggested change

raise ValueError(f'TypeSpecifier must be compatible with "mldataset" type specifier, was {type_specifier}')

raise ValueError(f'type_specifier must be compatible with "mldataset" type specifier, was {type_specifier}')

TypeSpecifier is not the argument's name, it is type_specifier.

forman · 2020-11-03T14:59:17Z

xcube/core/store/stores/memory.py

+        if type_specifier:
+            data_type_specifier = get_type_specifier(self._data_dict[data_id])
+            if not data_type_specifier.is_compatible(type_specifier):
+                raise ValueError(f'Data resource "{data_id}" is not available as type {type_specifier}. '


Suggested change

raise ValueError(f'Data resource "{data_id}" is not available as type {type_specifier}. '

raise ValueError(f'Data resource "{data_id}" is not compatible with type specifier "{type_specifier}". '

forman · 2020-11-03T15:12:25Z

xcube/core/store/store.py

-        If a store implementation supports only a single data type, it should verify that *type_id* is either None
-        or equal to that single data type.
+        If a store implementation supports only a single data type, it should verify that *type_specifier*
+        is either None or equal to that single data type.


Suggested change

is either None or equal to that single data type.

is either None or compatible with the supported data type.

forman · 2020-11-03T15:14:37Z

xcube/core/store/store.py

        """
        Get the descriptor for the data resource given by *data_id*.

-        Raises if *data_id* does not exist in this store.
+        Raises a DataStoreError if *data_id* does not exist in this store


Suggested change

Raises a DataStoreError if *data_id* does not exist in this store

Raises a :class:DataStoreError if *data_id* does not exist in this store

Not clear yet when we raise class:DataStoreError and when ValueError. For example in some modules, we raise ValueError if type_specifieris not compatible. That doesn't seem consistent to me.

forman · 2020-11-03T15:19:00Z

xcube/core/store/store.py

+        """
+        Get the tuple of data type specifiers that are supported for the given *data_id*.
+        In case the type specifier allows one ore more flags, they are listed in brackets
+        following the specifier's name, e.g., dataset[CUBE, MULTILEVEL].


Suggested change

following the specifier's name, e.g., dataset[CUBE, MULTILEVEL].

following the specifier's name, e.g., "dataset[cube,multilevel]".

forman · 2020-11-03T15:19:48Z

xcube/core/store/store.py

+        If *type_specifier* is omitted, all data resource identifiers are returned.
+
+        If a store implementation supports only a single data type, it should verify that *type_specifier*
+        is either None or equal to that single data type.


Suggested change

is either None or equal to that single data type.

is either None or compatible with the supported data type.

xcube/core/store/store.py

pont-us · 2020-11-04T09:58:11Z

test/core/store/test_descriptor.py

@@ -17,7 +17,7 @@ def test_from_dict_no_data_id(self):
        except ValueError:
            pass

-    def test_from_dict_wrong_type_id(self):
+    def test_from_dict_wrong_type_specifier(self):
        try:
            descriptor_dict = dict(data_id='xyz', type_id='tsr')


Suggested change

descriptor_dict = dict(data_id='xyz', type_id='tsr')

descriptor_dict = dict(data_id='xyz', type_specifier='tsr')

xcube/core/store/descriptor.py

pont-us · 2020-11-04T10:55:01Z

xcube/core/store/store.py


+        :param type_specifier: If given, only data identifiers that are available as this type are returned. If this is
+        omitted, all available data identifiers are returned


Suggested change

omitted, all available data identifiers are returned

omitted, all available data identifiers are returned.

pont-us · 2020-11-04T10:55:13Z

xcube/core/store/store.py


+        :param type_specifier: If given, only data identifiers that are available as this type are returned. If this is
+        omitted, all available data identifiers are returned
+        :param include_titles: If true, the store will attempt to also provide a title


Suggested change

:param include_titles: If true, the store will attempt to also provide a title

:param include_titles: If true, the store will attempt to also provide a title.

pont-us · 2020-11-04T12:08:37Z

xcube/core/store/typespecifier.py

    """
-    A type id denotes a type of data. It is used to group similar types of data and discern different types of data.
-    It can be used by stores to state what types of data can be read from and/or written to them.
+    A type specifier denotes a type of data. It is used to group similar types of data and discern


Suggested change

A type specifier denotes a type of data. It is used to group similar types of data and discern

A type specifier denotes a type of data. It is used to group similar types of data and distinguish between

I think this is clearer (hopefully I interpreted the intended meaning of "discern" correctly enough for a successful rephrasing...).

pont-us · 2020-11-04T12:29:25Z

xcube/core/store/stores/memory.py

-    def describe_data(self, data_id: str) -> DataDescriptor:
+        if not data_id in self._data_dict:
+            return False
+        if type_specifier:


Suggested change

if type_specifier:

if type_specifier is not None:

There are a few things (other than None) that may evaluate to False in this test, so I prefer an explicit None check. Admittedly it's unlikely that we'd be using e.g. 0 or the empty string as a type specifier, but if something like that is passed by mistake I think it's preferable to process it here (probably producing False or an error) rather than falling through to returning True.

pont-us · 2020-11-04T12:29:44Z

xcube/core/store/stores/memory.py

        self._assert_valid_data_id(data_id)
+        if type_specifier:


Suggested change

if type_specifier:

if type_specifier is not None:

See previous comment.

pont-us · 2020-11-04T13:51:01Z

docs/source/storeconv.md

+
+`<type_specifier>:<format_identifier>:<storage_identifier>`
+
+`<type_specifier>` MUST be a valid string that specifies a data type.


What's the definition of "valid" in this context?

pont-us · 2020-11-04T13:51:37Z

docs/source/storeconv.md

+`<type_specifier>:<format_identifier>:<storage_identifier>`
+
+`<type_specifier>` MUST be a valid string that specifies a data type.
+In case the type specifier has flags, the flags MUST be given in brackets, in alphabetic order, without spaces (e.g., `dataset[cube,multilevel]`).


Suggested change

In case the type specifier has flags, the flags MUST be given in brackets, in alphabetic order, without spaces (e.g., `dataset[cube,multilevel]`).

In case the type specifier has flags, the flags MUST be given in square brackets, in alphabetic order, separated by single commas, without spaces (e.g., `dataset[cube,multilevel]`).

pont-us · 2020-11-04T13:52:42Z

docs/source/storeconv.md

+
+`<type_specifier>` MUST be a valid string that specifies a data type.
+In case the type specifier has flags, the flags MUST be given in brackets, in alphabetic order, without spaces (e.g., `dataset[cube,multilevel]`).
+Note that `*` is a valid value in case that any type is supported.


Suggested change

Note that `*` is a valid value in case that any type is supported.

Note that `*` is a special value indicating that any type is supported.

forman · 2020-11-04T14:16:43Z

docs/source/storeconv.md

+
+`<type_specifier>:<format_identifier>:<storage_identifier>`
+
+`<type_specifier>` MUST be a valid string that specifies a data type.


Suggested change

`<type_specifier>` MUST be a valid string that specifies a data type.

`<type_specifier>` is a string that specifies a data type. Its intention and format is described below.

forman · 2020-11-04T14:21:02Z

docs/source/storeconv.md

+
+`<type_specifier>` MUST be a valid string that specifies a data type.
+In case the type specifier has flags, the flags MUST be given in brackets, in alphabetic order, without spaces (e.g., `dataset[cube,multilevel]`).
+Note that `*` is a valid value in case that any type is supported.


We want users to "note" everything in this doc. It is a (our) convention. So we define what things means and how they are used.

forman · 2020-11-04T14:22:36Z

docs/source/storeconv.md

+`<type_specifier>:<format_identifier>:<storage_identifier>`
+
+`<type_specifier>` MUST be a valid string that specifies a data type.
+In case the type specifier has flags, the flags MUST be given in brackets, in alphabetic order, without spaces (e.g., `dataset[cube,multilevel]`).


In case the type specifier has flags, the flags MUST be given in brackets...

This is our convention and we have to tell users about the usage and the general syntax of type specifiers. How can a user know if their specifier has flags or not, if it is not explained what these flags are and how they are used.

Provide a clear and unambiguous syntax descroiption (Backus Naur) and some explained examples.

forman · 2020-11-04T14:25:21Z

xcube/core/store/descriptor.py

-            raise ValueError(f'TypeId must be compatible with "dataset" type id, was {type_id}')
+    def _assert_type_specifier(self, type_specifier: str):
+        if not TYPE_SPECIFIER_DATASET.is_compatible(type_specifier):
+            raise ValueError(f'TypeSpecifier must be compatible with "dataset" type specifier, was {type_specifier}')


Suggested change

raise ValueError(f'TypeSpecifier must be compatible with "dataset" type specifier, was {type_specifier}')

raise ValueError(f'type_specifier must be compatible with "dataset" type specifier, was "{type_specifier}"')

forman · 2020-11-04T14:54:10Z

xcube/core/store/store.py

+    obtain in-memory representations. A data resource may be available as different types. Therefore, many functions
+    allow to specify the data type using a TypeSpecifier. A type specifier consists of a name and an arbitrary set of
+    flags, given in brackets. These flags are used to define characteristics of a type, e.g., the type specifier
+    dataset[cube] denotes a dataset which also meets the requirements of a cube. A dataset specified by


Suggested change

dataset[cube] denotes a dataset which also meets the requirements of a cube. A dataset specified by

"dataset[cube]" denotes a dataset which also meets the requirements of a cube. A dataset specified by

forman · 2020-11-04T14:55:09Z

xcube/core/store/store.py

+    allow to specify the data type using a TypeSpecifier. A type specifier consists of a name and an arbitrary set of
+    flags, given in brackets. These flags are used to define characteristics of a type, e.g., the type specifier
+    dataset[cube] denotes a dataset which also meets the requirements of a cube. A dataset specified by
+    dataset[cube, multilevel] is a cube and has multiple levels. A type specifier with a flag is compatible to a type


Suggested change

dataset[cube, multilevel] is a cube and has multiple levels. A type specifier with a flag is compatible to a type

"dataset[cube, multilevel]" is a cube and has multiple levels. A type specifier with a flag is compatible to a type

xcube/core/store/store.py

forman

Yeah!

TonioF added 2 commits November 2, 2020 16:51

edited store interface to better support type ids

ad8f85a

renamed type id to type specifier

2db9e95

TonioF requested review from forman and pont-us November 2, 2020 17:12

forman requested changes Nov 3, 2020

View reviewed changes

TonioF added 2 commits November 4, 2020 09:46

doc improvements after type specifier pr review

a29553b

explained type specifier in data store documentation

ec4b6bd

pont-us requested changes Nov 4, 2020

View reviewed changes

TonioF added 2 commits November 4, 2020 14:19

describe identifiers in storeconv

2462998

integrated pr review

6cd874a

pont-us requested changes Nov 4, 2020

View reviewed changes

forman self-requested a review November 4, 2020 14:12

forman requested changes Nov 4, 2020

View reviewed changes

TonioF added 4 commits November 5, 2020 09:27

use quotation marks in error messages

22f2c59

changed error types

31dd42d

put example values in quotation marks

76e3eef

updated naming identifiers section

780190e

TonioF requested review from forman and pont-us November 5, 2020 10:14

pont-us requested changes Nov 5, 2020

View reviewed changes

xcube/core/store/store.py Show resolved Hide resolved

forman approved these changes Nov 5, 2020

View reviewed changes

TonioF added 2 commits November 5, 2020 17:39

test fixes

e114823

ensure type specifier is written to dict as string

2b093a4

TonioF merged commit 33cd069 into master Nov 5, 2020

TonioF deleted the toniof-352-support-typespecifiers branch November 5, 2020 16:42

pont-us mentioned this pull request Nov 6, 2020

Update plugin to work with new xcube type specifier changes xcube-dev/xcube-cds#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toniof 352 support typespecifiers #354

Toniof 352 support typespecifiers #354

TonioF commented Nov 2, 2020

forman left a comment •

edited

Loading

forman Nov 3, 2020

TonioF Nov 4, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

forman Nov 3, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

pont-us Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman Nov 4, 2020

forman left a comment

	raise ValueError(f'TypeSpecifier must be compatible with "geodataframe" type specifier, '
	raise ValueError(f'type_specifier must be compatible with "geodataframe" type specifier, '

	raise ValueError(f'Data resource "{data_id}" is not available as type {type_specifier}. '
	raise ValueError(f'Data resource "{data_id}" is not compatible with type specifier "{type_specifier}". '

	is either None or equal to that single data type.
	is either None or compatible with the supported data type.

	Raises a DataStoreError if data_id does not exist in this store
	Raises a :class:DataStoreError if data_id does not exist in this store

	following the specifier's name, e.g., dataset[CUBE, MULTILEVEL].
	following the specifier's name, e.g., "dataset[cube,multilevel]".

	descriptor_dict = dict(data_id='xyz', type_id='tsr')
	descriptor_dict = dict(data_id='xyz', type_specifier='tsr')


		:param type_specifier: If given, only data identifiers that are available as this type are returned. If this is
		omitted, all available data identifiers are returned

	:param include_titles: If true, the store will attempt to also provide a title
	:param include_titles: If true, the store will attempt to also provide a title.

	A type specifier denotes a type of data. It is used to group similar types of data and discern
	A type specifier denotes a type of data. It is used to group similar types of data and distinguish between


		`<type_specifier>:<format_identifier>:<storage_identifier>`

		`<type_specifier>` MUST be a valid string that specifies a data type.

	Note that `*` is a valid value in case that any type is supported.
	Note that `*` is a special value indicating that any type is supported.

	`<type_specifier>` MUST be a valid string that specifies a data type.
	`<type_specifier>` is a string that specifies a data type. Its intention and format is described below.

	dataset[cube] denotes a dataset which also meets the requirements of a cube. A dataset specified by
	"dataset[cube]" denotes a dataset which also meets the requirements of a cube. A dataset specified by

	dataset[cube, multilevel] is a cube and has multiple levels. A type specifier with a flag is compatible to a type
	"dataset[cube, multilevel]" is a cube and has multiple levels. A type specifier with a flag is compatible to a type

Toniof 352 support typespecifiers #354

Toniof 352 support typespecifiers #354

Conversation

TonioF commented Nov 2, 2020

forman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forman left a comment

Choose a reason for hiding this comment

forman left a comment •

edited

Loading