CentreForDigitalHumanities / edpop-explorer

Common interface to multiple library catalogues and bibliographical databases
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

Added Pierre and Belle reader #30

Closed linguistcrg closed 3 months ago

linguistcrg commented 4 months ago

Added a reader for the dataset "BIBLIOGRAPHY OF EARLY MODERN EDITIONS OF PIERRE DE PROVENCE ET LA BELLE MAGUELONNE (CA. 1470–CA. 1800)."

tijmenbaarda commented 3 months ago

Great, this seems to work very well! I have some remarks but those can be addressed quickly I think.

You have created a reader that reads all records at once, no matter what is asked for. That is perfect here, because the dataset is very small. But you can indicate this by adding FETCH_ALL_AT_ONCE = True to the class definition. In addition, the start and end indices are now processed by the code, but in the end they are not used. The method fetch_range needs to accept the parameter range_to_fetch and return a range, because this method is overridden (so the parameters need to be the same as those of the parent class). But apart from that, nothing has to be done with it and the _perform_query method (which is not overridden) does not need the parameters start_record and maximum_records (start_record is not used at all and maximum_records is manipulated but after that not used). So you can simplify this still a bit - you could use fbtee.py as an example.

Also make sure that the CATALOG_URIREF and IRI_PREFIX attributes are defined. You can model them in the same way as other readers do - the URLs don't have to exist on the web.

I have some remarks concerning (some very small) details in the code.

linguistcrg commented 3 months ago

Thank you for your feedback! I just applied the changes you suggested. A few things to note:

tijmenbaarda commented 3 months ago

Excellent! I deleted the = file and I removed two unused imports, because the ruff linter in GitHub Actions was complaining.

linguistcrg commented 2 months ago

When I created the other readers, I realised that the Pierre Belle reader is case-sensitive. Should I modify this, or is it not an important feature?