DRAFT: Upgraded OPDS 2.0 Version of Autocat - #232
Conversation
… to old custom crosswalk setting.
…y code/comment formatting (used rufus). centralized some url logic in opds.
…lves and locc results that refreshes every 24 hours.
|
For now this is WIP since it will not be launched through ibiblio for now but rather pglaf and yet to run on prod. |
cpeel
left a comment
cpeel
left a comment
There was a problem hiding this comment.
I had some challenges with loading the extension in the materialized view SQL due to what I think is an oddity in the gutenberg database. After you pull that file into the PR (per one of my comments) we can iterate over that.
Co-authored-by: Casey Peel <cpeel@users.noreply.github.com>
Co-authored-by: Casey Peel <cpeel@users.noreply.github.com>
… been formatted to be more readable. I also removed the copyrighted facet from the OPDS feed and limited the langauge facet to the most popular ones for now.
…ld var. Added default error page to CherrPyApp for unforseen errors.
…speed. Not every query is requried to be counted now for some performance cases (like loading homepage). Cached the counts for LoCC browsing section (refreshes two times a day) for faster load.
| pghost: '127.0.0.1' # to connect to a local postgres | ||
| pgport: 5432 | ||
| pgdatabase: 'gutenberg' | ||
| pgdatabase: 'gutenebrg' |
…server. everything loads instant now.
…itle, and author as fields available to restrict search by. Updated tests accordingly.
…eaningful speed enhancements.
…equest self link they get full, but on home screen for example they get the opds_small crosswalk. I refactored crosswalks.py to use more helpers so that it is easier to read and maintain. I also updated the OPDS config to use gzip when sending in both the dev/prod version. I also updated CherrPy.conf to zip any json we currently send or will send in the future. The homepage alone went from 638kb to now just 13kb.
…facet logic into one function within Search.py. I pinned your selected subject facet so when you filter on language after selecting a subject it doesnt get "lost".
|
I highly suggest adopting the 50/72 rule for commit messages. That will make your commit history much easier to follow. |
|
@zachjesus - I'm going to leave this feedback here rather than a Slack DM so @eshellman and @tangledhelix can chime in based on their experience too. Let's talk about collaboration in PRs. First and foremost, a PR is a request on another developer's (or multiple developers') time to review our code and help us spot problems and think about edgecases. Depending on the change, a reviewing developer may check your branch out and run it. An experienced developer will try to load the code into a mental model to understand what's going on, look for security issues, performance issues, vulnerabilities, and the usual best practices. To that end, it's key that we make the best of use of their time and make it as easy to review as possible. We often open a not-quite-ready PR to make people aware of it. That could be marked by a WIP (like you used) or putting it in a draft mode. Ideally, if it's in this state, the description will talk some about why it's in this state (needs more testing, need to refactor something, etc). After it's out of the WIP/draft mode, there's an implied agreement that the PR won't have substantial changes unless they are to address things found in the PR. In particular, it means that the PR has been tested, validated, and any refactors have already been done. Because big changes require the reviewer to look at it all again, try to recreate the context of what's going on and what has changed, and maybe even re-test it. If, during a code review, we discover that it needs a refactor or there are performance problems it's usually resolved in one of two ways:
In particular for major new features like this, we want to focus on the minimal viable product (MVP) that gets it working. Review it. Merge it. And then have follow-up PRs that focus on performance, improvements, etc. Different teams do it differently and we don't have a community standard for PG but this is how I approach PRs. |
|
I agree with most or all of what Casey says here. A couple other comments below.
...
Time is the key word in both of these thoughts; I won't write an essay on that here, but suffice it to say that volunteer time can be hard to come by, and it's something all of us should try to keep in mind.
Yes, please - WIP or Draft until it is ready. If it's not marked as such, I will assume I am reviewing something the author considers ready to ship. Dan |
|
Thanks Casey and Dan. These are good comments. Zach and I discussed this today. |
…If no author is listed in our metadata, but some other creator(s) exists, the primary contributor is listed as the author in the OPDS feed. They will also be listed as their proper role below matching the OPDS schema. If no author, then the name Anonymous is used. This change is trying to be considerate to OPDS clients and simplify all the nuances in displaying a book for them. (fix/OPDS2): Clicking on LoCC subject tag now routes to search?locc= instead of the locc route to avoid the case when a code displays children navigations not publications.
… matched to Readium spec as close as possible.
…to display. (fix/opds) Always include aquisiton link for book even when its just a preview.
… only browse subjects but bookshelves.
…okshelf groups to navs.
…icient. (fix/opds2) fixed some queries from pulling more data then they needed to.
Various fixes requested by Hadrien from EDR Labs. Includes testing changes requested by Eric.
zachjesus
changed the title
|
Once I get final approval from EDR Labs, the designers of OPDS standard, I will remove the draft status of this pull. The last commits were targeted to clean the issues they had with the feed. So, after this, we should (fingers crossed) be in the clear. |
4 participants
This code contains all the code required to launch the new OPDS 2.0 server. The readme explains what has been added and how to run this new version.