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.

Sign up for GitHub

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

Jump to bottom

JSON-LD Metadata for JSON API #92

Merged
merged 14 commits into from
Feb 3, 2025
Merged

JSON-LD Metadata for JSON API #92

merged 14 commits into from
Feb 3, 2025

Conversation

khalsz
Copy link
Collaborator

@khalsz khalsz commented Nov 7, 2024

Here is a demo result of the signal database query.

image

@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch 2 times, most recently from dd0f275 to b3409e0 Compare November 13, 2024 12:30
@khalsz
Add: raw string 'r' to dim_names regex
27bdf83 
Add: json-ld mapping details to model.py file

fix: edit context, type, and other tables keys in main.py file

Add: columns Location, Rights, Language and Format to all tables

Fix: reposition context  and type in the response dictionary

Fix: column names in test data

Fix: individual item query bug

Fix: cpf file path bug

Fix: CPF_summary error (remove duplicate values)

Merge with main branch
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from 4fc1525 to 27bdf83 Compare November 14, 2024 14:32
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from ea81da3 to db15fc3 Compare November 14, 2024 14:54
@khalsz khalsz marked this pull request as ready for review November 15, 2024 12:05
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from 191e881 to f78c0cc Compare November 15, 2024 12:09
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from f78c0cc to 67c8136 Compare November 15, 2024 12:20
@samueljackson92 samueljackson92 changed the title Feat/t #77 jsonld mapping JSON-LD Metadata for JSON API Nov 19, 2024
samueljackson92
samueljackson92 requested changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@samueljackson92 samueljackson92 left a comment

Choose a reason for hiding this comment

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

Generally looks good, a couple of minor questions.

Respond and fix these and then I'll run it locally and take a look at the output.

src/api/main.py Outdated Show resolved Hide resolved
src/api/models.py Outdated Show resolved Hide resolved
src/api/models.py Outdated Show resolved Hide resolved
src/api/models.py Outdated Show resolved Hide resolved
src/api/models.py Outdated Show resolved Hide resolved
@NathanCummings
Copy link
Member

In addition to Sam's comments, would it not make more sense to leave out some of these terms that we don't have an entry for yet? For example, dct:format and dct:location. Until we have this information in our metadata, I don't think the placeholders are necessary.

@khalsz
Copy link
Collaborator Author

khalsz commented Nov 25, 2024

The specific corrections mades in the comments have been addressed. Please review.

samueljackson92
samueljackson92 requested changes Nov 25, 2024
View reviewed changes
src/api/main.py Outdated Show resolved Hide resolved
src/api/main.py Outdated Show resolved Hide resolved
@NathanCummings
Copy link
Member

Could you also pull the CI updates in to this branch please?

NathanCummings
NathanCummings reviewed Nov 27, 2024
View reviewed changes
src/api/models.py Outdated Show resolved Hide resolved
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from c63ccb8 to 7d3d25c Compare November 27, 2024 16:23
@khalsz khalsz force-pushed the feat/T-#77-jsonld-mapping branch from 7d3d25c to 3e6bc1a Compare December 16, 2024 14:50
@agbeltran agbeltran self-assigned this Jan 7, 2025
samueljackson92
samueljackson92 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@samueljackson92 samueljackson92 left a comment

Choose a reason for hiding this comment

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

I think this looks good to me, but I would be nice to get @agbeltran to check that the outputs do indeed make sense and we're not abusing the terms used.

image

samueljackson92
samueljackson92 requested changes Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@samueljackson92 samueljackson92 left a comment

Choose a reason for hiding this comment

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

@khalsz this PR has merge conflicts. Also I've just spotted that the titles should be more generic that "MAST-U Source". Maybe "Diagnostics source" instead?

@samueljackson92 samueljackson92 merged commit 032e1c4 into main Feb 3, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanCummings NathanCummings NathanCummings left review comments

@samueljackson92 samueljackson92 samueljackson92 approved these changes

Assignees

@agbeltran agbeltran

Labels
None yet
Projects
None yet
Milestone
FCL Workshop Winter 2025
Development

Successfully merging this pull request may close these issues.
4 participants
@khalsz @NathanCummings @samueljackson92 @agbeltran