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

Unfreeze PyMongo and replace deprecated count() with count_documents() #124

Merged
merged 20 commits into from
Oct 27, 2023

Conversation

northwestwitch
Copy link
Member

@northwestwitch northwestwitch commented Oct 25, 2023

This PR adds | fixes:

  • Unfreezed PyMongo in requirements.txt
  • Replaced deprecated pymongo .count() function with count_documents() in code.
  • Added basic cli tests touching fixed deprecated code

How to test:

  • Automatic tests should pass

Expected outcome:

  • All tests should pass

Review:

  • Code approved by HS
  • Tests executed by CR

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@northwestwitch northwestwitch marked this pull request as ready for review October 25, 2023 11:56
Copy link
Contributor

@henrikstranneheim henrikstranneheim left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Test with a relevant CLI cmd perhaps?

@northwestwitch
Copy link
Member Author

Looks good 👍 Test with a relevant CLI cmd perhaps?

That's a good idea, I'll fix tomorrow. Thanks for the review!

@northwestwitch northwestwitch marked this pull request as draft October 27, 2023 06:30
@northwestwitch
Copy link
Member Author

northwestwitch commented Oct 27, 2023

Looks good 👍 Test with a relevant CLI cmd perhaps?

Mmm I've changed the automatic tests to run with Mongo 7 and a new version of PyMongo, but I see that there are some deprecated methods in the code that shouldn't be supported in the older versions of pymongo. Perhaps these are not covered by the tests. I'll fix!

@northwestwitch northwestwitch changed the title Unfreeze PyMongo Unfreeze PyMongo and replace deprecated count() with count_documents() Oct 27, 2023
@northwestwitch northwestwitch marked this pull request as ready for review October 27, 2023 08:33
@northwestwitch
Copy link
Member Author

Ok, now I feel thsi ready for a review @henrikstranneheim, thanks!

Copy link
Contributor

@henrikstranneheim henrikstranneheim left a comment

Choose a reason for hiding this comment

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

Great that you added tests 👍

loqusdb/commands/identity.py Outdated Show resolved Hide resolved
loqusdb/commands/view.py Outdated Show resolved Hide resolved
loqusdb/commands/view.py Outdated Show resolved Hide resolved
result = adapter.get_clusters(variant_id)
if result.count() == 0:
nr_results = adapter.db.identity.count_documents({"variant_id": variant_id})
if nr_results == 0:
LOG.info("No hits for variant %s", variant_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

f-str


from loqusdb.commands.cli import cli as base_command

def test_export_base(real_db_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add type hint


## THEN it should return success
result = runner.invoke(base_command, command)
assert result.exit_code == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also test that it actually exported a file?

Copy link
Member Author

@northwestwitch northwestwitch Oct 27, 2023

Choose a reason for hiding this comment

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

If you don't provide the -o param in the cli is not exporting to file, but printing into the terminal:

image

For the purpose of testing that the are 0 cases with the modified count command I think this is good enough.
But I agree that more and better tests for the cli could have been written!


from loqusdb.commands.cli import cli as base_command

def test_identity(real_db_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add type hint


from loqusdb.commands.cli import cli as base_command

def test_view_cases_base(real_mongo_adapter, real_db_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add type hints

tests/commands/test_view.py Show resolved Hide resolved
northwestwitch and others added 6 commits October 27, 2023 13:19
Co-authored-by: Henrik Stranneheim <henrik.stranneheim@scilifelab.se>
Co-authored-by: Henrik Stranneheim <henrik.stranneheim@scilifelab.se>
@northwestwitch northwestwitch merged commit 2043c68 into master Oct 27, 2023
2 checks passed
@northwestwitch northwestwitch deleted the unfreeze_pymongo branch October 27, 2023 12:02
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.

2 participants