-
Notifications
You must be signed in to change notification settings - Fork 6
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
63 google dataset search support #136
base: main
Are you sure you want to change the base?
Conversation
should this be a route or..? right now it gets printed to stdout everytime the datasets are displayed but needs to be inserted into the frontend
I'll work that up before next mtg, and check on dataset page placement
get_dataset_jsonld route, off of get_dataset now, so can get just one for the frontend rendering of that one dataset page but the problem is this method gets a dict from mongo that has objids & datatimes which are not serializable, as originally needed
back to easy pydantic dso.json() mapping
ret script str; and /jsonld to differentiate route
remove prints
try summary.jsonld in case we want a summary.json (which I would use, and just map) also added /sitemap.xml that dumps (in case we want to cache it) then returns it but none of it's urls can be crawled yet, till v2 gets a public setting
sitemap() -> str: vs response_model
used black to get past lint test
took hack advice to make /summary.jsonld and /sitemap.xml open, but not working yet
@@ -116,6 +116,16 @@ | |||
tags=["extractors"], | |||
dependencies=[Depends(get_current_username)], | |||
) | |||
api_router.include_router( | |||
extractors.router, |
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.
Your newly added routes are in routers/datasets.py; but this new prefix is importing routes from extractors.
So i can actually access your routes under the /dataset/{id}/summary.jsonld
but not here
I'd recommend you separate them out in a new router file (e.g. summary.py) and include them here. summary.router
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.
that was left over, thanks, will skip the tags for now, and take your other advice to put those routes in another file
@@ -116,6 +116,16 @@ | |||
tags=["extractors"], | |||
dependencies=[Depends(get_current_username)], | |||
) | |||
api_router.include_router( | |||
extractors.router, | |||
prefix="/summary.jsonld", |
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.
prefix is for the prefix :-p
e.g. prefix = "/dataset" means the routes will all start with /dataset s--> /datasets/{id}, /datasets/{id}/files etc
backend/app/routers/datasets.py
Outdated
@@ -226,6 +270,56 @@ async def get_dataset(dataset_id: str, db: MongoClient = Depends(dependencies.ge | |||
raise HTTPException(status_code=404, detail=f"Dataset {dataset_id} not found") | |||
|
|||
|
|||
@router.get("/{dataset_id}/summary.jsonld", response_model=DatasetOut) |
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 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.
during a cleanup I make sure it is a DatasetOut to safe to call .json on it now
backend/app/routers/datasets.py
Outdated
return f.read() | ||
|
||
|
||
@router.get("/sitemap.xml") |
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 does not work... when i try /datasets/sitemap.xml
it try to cast sitemap.xml into an objectId, I'm guessing because it collides with the /datasets/{id}
endpoint
if you separate these out to separate file and give different prefix than dataset, it might 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.
separated out into a sitemap.py file, &will make another pass soon, to see if it will open up the route
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.
different file now, w/no overlapping routes
took out old/overlapping routes, and check if DatasetOut to make sure can call .json on it
issue63 early functions and route for getting /summary.jsonld
that can be inserted in the dataset pages for crawling via reading
a /sitemap.xml route, but will need to be able to make those urls public 1st
Could go right from sitemap to summary.jsonld urls till can get in dataset page