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

#911 dcat distribution model fix inspect api updates #912

Merged

Conversation

SarahJohnsonONS
Copy link
Contributor

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 of writers/helpers/qbwriter/dsdtordfmodelshelper.py).

Copy link

github-actions bot commented Jun 6, 2024

ubuntu-latest-python3.11-pandas~2.0 test results

 10 files  ±0   10 suites  ±0   4m 36s ⏱️ +12s
589 tests ±0  589 ✅ ±0  0 💤 ±0  0 ❌ ±0 
590 runs  ±0  590 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0249d1e. ± Comparison against base commit 2fcf99c.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 6, 2024

ubuntu-latest-python3.11-pandas@^1.3 test results

 10 files  ±0   10 suites  ±0   4m 36s ⏱️ +13s
589 tests ±0  589 ✅ ±0  0 💤 ±0  0 ❌ ±0 
590 runs  ±0  590 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0249d1e. ± Comparison against base commit 2fcf99c.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 6, 2024

ubuntu-latest-python3.9- test results

 10 files   10 suites   4m 38s ⏱️
589 tests 589 ✅ 0 💤 0 ❌
590 runs  590 ✅ 0 💤 0 ❌

Results for commit 0249d1e.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 6, 2024

windows-latest-python3.11-pandas~2.0 test results

 11 files  ±0   12 suites  ±0   6m 21s ⏱️ -28s
603 tests ±0  603 ✅ ±0  0 💤 ±0  0 ❌ ±0 
617 runs  ±0  617 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0249d1e. ± Comparison against base commit 2fcf99c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@canwaf canwaf left a comment

Choose a reason for hiding this comment

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

Review WIP

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@canwaf canwaf left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Flag for review

Copy link
Contributor Author

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.

title: Annotated[
str, Triple(DCTERMS.title, PropertyStatus.recommended, map_str_to_en_literal)
]
has_primary_source: Annotated[
Copy link
Contributor

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?

Copy link
Contributor Author

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]


Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Raise this ticket please.

Copy link
Contributor

@canwaf canwaf left a 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.

Copy link
Contributor

@canwaf canwaf left a 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.

src/csvcubed/models/inspect/sparqlresults.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jul 17, 2024

@SarahJohnsonONS SarahJohnsonONS merged commit be725e5 into main Jul 17, 2024
16 checks passed
@SarahJohnsonONS SarahJohnsonONS deleted the #911-dcat-distribution-model-fix-inspect-API-updates branch July 17, 2024 09:43
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