Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

One-time version check #1456

Merged
merged 17 commits into from
Jul 18, 2024
Merged

One-time version check #1456

merged 17 commits into from
Jul 18, 2024

Conversation

rzats
Copy link
Contributor

@rzats rzats commented May 29, 2024

Closes #1281

Summary:

Adds a one-time check to the Epidata client, which logs a warning when the current "remote" server version (https://api.delphi.cmu.edu/epidata/version) does not match the client version.

This is limited to debug mode for now but can be easily extended to run by default.

Also adds a test to verify this behavior.

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

@rzats rzats requested review from melange396 and dshemetov May 29, 2024 15:28
@melange396
Copy link
Collaborator

Let's do #1296 first and then you can revisit this

@rzats
Copy link
Contributor Author

rzats commented Jun 25, 2024

@melange396 since #1296 is merged, we can move forward with this PR; lmk if the current iteration looks good!

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Do yall only need this for internal use? If so, then this is good to go. If not, then we should not hide the version check behind a debug flag.

rzats and others added 4 commits July 10, 2024 00:09
Co-authored-by: Dmitry Shemetov <dshemetov@ucdavis.edu>
Co-authored-by: Dmitry Shemetov <dshemetov@ucdavis.edu>
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

This functionality has enough purpose (and code!) for it to be in its own method and not taking up >50% of the lines inside the _request() method.

The intent is for this to be helpful to the user -- without their needing to ask for it -- thus it would be better for the flag/argument to be opt-out instead of opt-in, if we give the option at all.

I had envisioned that we would do this on module load [only once, on the initial load into the namespace]. That might make it hard to do opt-in/out behavior, but it could make for a more natural interaction with the user, especially if they are using it in an interactive environment. What do you think?

)
except Exception as e:
Epidata.log("Error getting latest client version", exception=str(e))
_version_check.__func__() # run this once on module load
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Python <= 3.9, static methods cannot be directly called within the class body.
In Python >= 3.10, it works as you'd expect: _version_check()

Copy link
Contributor

@dshemetov dshemetov Jul 15, 2024

Choose a reason for hiding this comment

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

Suggested change
_version_check.__func__() # run this once on module load
# Run this once on module load. Use dunder method for Python <= 3.9 compatibility
# https://stackoverflow.com/a/12718272
_version_check.__func__()

Copy link
Collaborator

Choose a reason for hiding this comment

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

plz also add newline above function call!

would it be any better/easier to avoid this pattern here and just call Epidata._version_check() at the end of the file (after the class definition)?

@rzats rzats requested review from dshemetov and melange396 July 15, 2024 13:33
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

plz also add an entry to CHANGELOG!

)
except Exception as e:
Epidata.log("Error getting latest client version", exception=str(e))
_version_check.__func__() # run this once on module load
Copy link
Collaborator

Choose a reason for hiding this comment

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

plz also add newline above function call!

would it be any better/easier to avoid this pattern here and just call Epidata._version_check() at the end of the file (after the class definition)?

@rzats rzats requested a review from melange396 July 18, 2024 11:41
Co-authored-by: Dmitry Shemetov <dshemetov@ucdavis.edu>
Copy link

@melange396 melange396 removed the request for review from dshemetov July 18, 2024 14:23
@melange396 melange396 requested a review from dshemetov July 18, 2024 14:24
@melange396 melange396 dismissed dshemetov’s stale review July 18, 2024 14:26

concerns addressed

@melange396 melange396 merged commit 35d67a7 into dev Jul 18, 2024
7 checks passed
@melange396 melange396 deleted the rzatserkovnyi/client-version-checking branch July 18, 2024 14:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client version checking
3 participants