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

Small cleanups to List models #6073

Merged
merged 5 commits into from
Mar 7, 2022

Conversation

cdrini
Copy link
Collaborator

@cdrini cdrini commented Jan 25, 2022

Refactor; remove some dead/broken code from List models. Includes a small breaking change to public APIs.

Technical

  • Remove some dead functions
  • Use Python 3.8's cached_property instead of home grown
  • Fixed + Re-enabled tests for this file
  • Removed dead list/seed properties: work_count, edition_count, ebook_count
    • These values were always 0. Note this is a breaking change to a public API, but considering it was not working and that fixing any broken code is trivial (switch to seed_count), I think it's ok.

Testing

  • Locally tested:
    • Lists with works render
      • Lists render
      • .json works
      • seeds.json works
      • All the export options work
    • Lists with subjects/authors:
      • Lists render
      • .json works
      • seeds.json works
      • None of the export options work -- this is the case in prod
  • Test on real data on testing
    • Test something with redirects

Screenshot

Stakeholders

- Remove some dead functions
- Use Python 3.8's cached_property instead of home grown
    - Confirmed this behaves in a similar fashion; see https://docs.python.org/3.9/library/functools.html#functools.cached_property
- Fixed + Re-enabled tests for this file
…ount

These values were always 0. Note this is a breaking change to a public API, but considering it was always broken and that fixing any broken code is trivial (switch to seed_count), I think it's ok.
@cdrini cdrini added the Theme: Lists Issues related to reading Lists label Jan 25, 2022
@mekarpeles mekarpeles added the Priority: 2 Important, as time permits. [managed] label Feb 14, 2022
@mekarpeles
Copy link
Member

changes lgtm, some concerns about whether there are other pages using any of the functionality that we've changed (apis, list page, etc). This is @jimchamp's PR to test but we wanted to look through it as we are also working on ListQuery Carousel changes and wanted to avoid overlap

@mekarpeles mekarpeles added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] and removed Priority: 2 Important, as time permits. [managed] labels Feb 22, 2022
@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Mar 1, 2022
@mekarpeles mekarpeles assigned mekarpeles and unassigned jimchamp Mar 7, 2022
@mekarpeles mekarpeles merged commit 8b50277 into internetarchive:master Mar 7, 2022
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Do this week, receiving emails, time sensitive, . [managed] Theme: Lists Issues related to reading Lists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants