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

convert args to kwargs to reduce nb of unique keys #140

Merged
merged 6 commits into from
Feb 2, 2024
Merged

convert args to kwargs to reduce nb of unique keys #140

merged 6 commits into from
Feb 2, 2024

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Feb 1, 2024

resolves #138, see details in the issue

cc: @shaypal5

@Borda Borda marked this pull request as draft February 1, 2024 09:32
@Borda Borda marked this pull request as ready for review February 1, 2024 11:49
@shaypal5
Copy link
Collaborator

shaypal5 commented Feb 1, 2024

Hey @Borda,

Please note that your code fails some non-mongo-core-related tests.
Once you only fail mongo-core-related tests, I can pull this into a dev branch to run against a live MongoDB instance, to make sure such failures are not due to running against an in-memory mock of MongoDB.

@Borda
Copy link
Contributor Author

Borda commented Feb 1, 2024

Please note that your code fails some non-mongo-core-related tests.

yes, looking into it... in addition, I may have two suggestions

  • run mongo server as part of the job so that all forked PRs will be tested
  • I see you also have pytest markers, so how about splitting the testing into two steps pytest tests/ -m "not mongo" and pytest tests/ -m "mongo" so it is easier to spot the issue (fine to send it in separate PR ci: split tests for local / db #142 🐰)

@shaypal5
Copy link
Collaborator

shaypal5 commented Feb 1, 2024

Please note that your code fails some non-mongo-core-related tests.

yes, looking into it... in addition, I may have two suggestions

  • run mongo server as part of the job so that all forked PRs will be tested
  • I see you also have pytest markers, so how about splitting the testing into two steps pytest tests/ -m "not mongo" and pytest tests/ -m "mongo" so it is easier to spot the issue (fine to send it in separate PR ci: split tests for local / db #142 🐰)
  1. Nice. Waiting for updates on resolution of these bugs.
  2. Sounds good. A nice future to-do.
  3. Sounds good! :)

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a68b3a) 99.40% compared to head (a0401aa) 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #140      +/-   ##
===========================================
+ Coverage   99.40%   100.00%   +0.59%     
===========================================
  Files           5         5              
  Lines         508       511       +3     
  Branches       88        87       -1     
===========================================
+ Hits          505       511       +6     
+ Misses          2         0       -2     
+ Partials        1         0       -1     
Files Coverage Δ
cachier/core.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a68b3a...a0401aa. Read the comment docs.

@Borda
Copy link
Contributor Author

Borda commented Feb 1, 2024

@shaypal5 seems to be passing now... 🐿️

@shaypal5
Copy link
Collaborator

shaypal5 commented Feb 1, 2024

Great! Let's finish with the test cleanup PRs first, then you can rebase over them and we can get cleaner tests results; we'll move directly to vs-real-mongo testing after that.

@Borda Borda requested a review from shaypal5 February 2, 2024 10:59
@Borda Borda changed the base branch from master to dev/args-kwds February 2, 2024 11:36
@Borda Borda merged commit ffd6312 into python-cachier:dev/args-kwds Feb 2, 2024
10 of 18 checks passed
@Borda Borda deleted the args-kwargs branch February 2, 2024 11:36
shaypal5 pushed a commit that referenced this pull request Feb 2, 2024
* add testing for identical inputs
* ignore local IDE files
* converting args to kwargs
* wrapped funx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert args to kwargs -> reduce keys for identical inputs
2 participants