-
Notifications
You must be signed in to change notification settings - Fork 1
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
#911 dcat distribution model fix inspect api updates #912
#911 dcat distribution model fix inspect api updates #912
Conversation
ubuntu-latest-python3.9- test results 10 files 10 suites 4m 38s ⏱️ Results for commit 0249d1e. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review WIP
...i/inspect/multi-attribute-resource-values/multi-attribute-duplicate-labels.csv-metadata.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a deperciation warning about old style csvws generated using the 0.1.0-dev1 output/models, and that only the new relational structure will eventually be supported. Further communiucaton at a later date.
dcat_dataset_with_metadata = rdf.dcat.Dataset(self._uris.get_dataset_uri()) | ||
self.cube.metadata.configure_dcat_dataset(dcat_dataset_with_metadata) | ||
dcat_dataset_with_metadata.distribution.add( | ||
ExistingResource(self._uris.get_distribution_uri()).uri # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag for review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to fix this but I have no idea what's going on, so I'll add it as a new ticket.
...cli/inspect/multi-attribute-resource-values/multi-attribute-duplicate-uris.csv-metadata.json
Outdated
Show resolved
Hide resolved
...i/inspect/multi-attribute-resource-values/multi-attribute-duplicate-labels.csv-metadata.json
Outdated
Show resolved
Hide resolved
...cli/inspect/multi-attribute-resource-values/multi-attribute-duplicate-uris.csv-metadata.json
Outdated
Show resolved
Hide resolved
...i/inspect/multi-attribute-resource-values/multi-attribute-duplicate-labels.csv-metadata.json
Outdated
Show resolved
Hide resolved
tests/test-cases/cli/inspect/multi-unit_multi-measure/alcohol-bulletin.csv-metadata.json
Show resolved
Hide resolved
tests/test-cases/cli/inspect/multi-unit_multi-measure/alcohol-bulletin.csv-metadata.json
Outdated
Show resolved
Hide resolved
...li/inspect/multi-unit_single-measure/new/multi-unit-single-measure-dataset.csv-metadata.json
Outdated
Show resolved
Hide resolved
...li/inspect/multi-unit_single-measure/new/multi-unit-single-measure-dataset.csv-metadata.json
Show resolved
Hide resolved
title: Annotated[ | ||
str, Triple(DCTERMS.title, PropertyStatus.recommended, map_str_to_en_literal) | ||
] | ||
has_primary_source: Annotated[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this show up in here? Should this not be in models?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prov.py
was already here (with the Activity
class definition). Rob did tell me why some of the models are here instead of in csvcubed-models, but I can't remember exactly what he said - something about it being more flexible?
@@ -616,3 +629,22 @@ def map_row(row_result: Dict[str, Any]) -> ColumnDefinition: | |||
) | |||
|
|||
return [map_row(row.asdict()) for row in sparql_results] | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you use black to lint this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
distribution.identifier = self.get_identifier() | ||
distribution.creator = self.creator_uri | ||
distribution.landing_page = set(self.landing_page_uris) | ||
distribution.themes = set(self.theme_uris) | ||
distribution.keywords = set(self.keywords) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna need you to tell me how we populate the wasGeneratedBy bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In writers/helpers/qbwriter/dsdtordfmodelshelper.py
L280.
@@ -97,13 +97,28 @@ def get_csvw_type_str(csvw_type: CSVWType) -> str: | |||
else: | |||
raise InputNotSupportedException() | |||
|
|||
def get_build_minor_version(self) -> int: | |||
"""Return the minor version of csvcubed used to build the cube.""" | |||
# TODO Create a class to store csvcubed version information similar to readers/cubeconfig/schema_versions.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raise this ticket please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a phenominal amount of work, and a great bit of effort. There are no clangers here, and nothing massively needing rework. We can pretty much have all our test cases (behaviour) align to the new object model, and relegate successful inspects on a single test case generated by a previous version of csvcubed.
I will need you to sort out the object model issues I have raised, resolve all the comments appropriately and I can approve.
Excellent work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one change you need to make, which is commented; however you can merge after you make that change.
Quality Gate passedIssues Measures |
Cubes now being built according to requirements here: #911. Backwards-compatible, so cubes built using version < 0.5.0 can still be inspected.
Unit and behaviour tests are passing. There was a niggly little issue with pyright, which I've fixed temporarily using
type: ignore
(on L329 ofwriters/helpers/qbwriter/dsdtordfmodelshelper.py
).