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

Rewrite the get_publication script #832

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Conversation

ptgolden
Copy link
Member

@ptgolden ptgolden commented Oct 9, 2024

This overhauls the script to update publications from Google Scholar. The previous script worked, but it had some drawbacks, namely that it required manually editing JSON with updated information. The new script does not, and it outputs data in a format that will create meaningful diffs with the existing file format when data is updated.

Changes:

  • Moves from argparse to typer for dealing with CLI arguments.

  • Separates fetching data from generating the publications.json file (this was necessary for development to prevent having to hit Google Scholar on every change).

  • Removes need to filter publications based on year-- Google Scholar is now taken as the single source of truth for publications.

  • Takes into account publications whose years have changed in Google Scholar versus what has been already included in the publications.json file.

  • Better record matching that ignores differences in non-alphanumeric characters. Previously, there were several false positives that had to do with changes in punctuation.

  • Logging to stderr rather than an outputted report.

  • Single source of truth for adding links to publications without them in Google Scholar. (Previously, they had to be input by hand in the produced JSON file).

This commit overhauls the script to update publications from Google
Scholar. The previous script worked, but it had some drawbacks, namely
that it required manually editing JSON with updated information. The new
script does not, and it outputs data in a format that will create
meaningful diffs with the existing file format when data is updated.

Changes:

  * Moves from argparse to typer for dealing with CLI arguments.

  * Separates fetching data from generating the publications.json file
    (this was necessary for development to prevent having to hit Google
    Scholar on every change).

  * Removes need to filter publications based on year-- Google Scholar
    is now taken as the single source of truth for publications.

  * Takes into account publications whose years have changed in Google
    Scholar versus what has been already included in the
    publications.json file.

  * Better record matching that ignores differences in non-alphanumeric
    characters.  Previously, there were several false positives that had
    to do with changes in punctuation.

  * Logging to stderr rather than an outputted report.

  * Single source of truth for adding links to publications without them
    in Google Scholar. (Previously, they had to be input by hand in the
    produced JSON file).
Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for monarch-app ready!

Name Link
🔨 Latest commit 4df0c1e
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/6716c6344bd5b600084bb70f
😎 Deploy Preview https://deploy-preview-832--monarch-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.30%. Comparing base (2c878e8) to head (4df0c1e).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
+ Coverage   70.75%   71.30%   +0.54%     
==========================================
  Files          91       91              
  Lines        3084     3136      +52     
==========================================
+ Hits         2182     2236      +54     
+ Misses        902      900       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sagehrke sagehrke left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks so much for doing this overhaul to make our lives easier!
My only minor update is the titles of the page and each section are now left justified in the deploy preview. Once that is put back to centered, I think it looks good to go!

@ptgolden
Copy link
Member Author

Hmm. Given that these changes don't touch any of the presentation code, that's a separate issue. I imagine it will require some changes in https://github.com/monarch-initiative/monarch-app/blob/2c878e844bc2d10664d798570560c04fad6acc27/frontend/src/components/AppCitation.vue. I'll check it out. I wonder why it only just popped up.

@kevinschaper
Copy link
Member

Is this ready to be merged? @sagehrke @ptgolden

@sagehrke
Copy link
Member

The deploy preview still shows issues. I am not sure if there is somewhere else I should be looking for the corrections to the left align issue and the tile spreading issue? @ptgolden

@sagehrke
Copy link
Member

#829 looks correct and related to this PR as well.

@kevinschaper
Copy link
Member

The layout in the deploy preview matches what's on dev now, so bringing this in is probably neutral relative to the layout of the page. That fix is coming before we release, so I think it makes sense to merge this

@kevinschaper kevinschaper merged commit 226062c into main Oct 21, 2024
11 checks passed
@kevinschaper kevinschaper deleted the update-publications-rework branch October 21, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants