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

remove 1to2 and depreciations #214

Merged
merged 3 commits into from
Apr 2, 2024
Merged

Conversation

andrewgsavage
Copy link
Contributor

Here I've removed:

  • the 1to2 code,
  • the compatability code that handled obselete function calls,
  • the depreciations in these calls,
  • the tests that tested obselete behaviour

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.75%. Comparing base (e9b82f0) to head (d963b9b).

❗ Current head d963b9b differs from pull request most recent head 53bf95a. Consider uploading reports for the commit 53bf95a to get more accurate results

Files Patch % Lines
uncertainties/unumpy/core.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   95.53%   95.75%   +0.21%     
==========================================
  Files          17       11       -6     
  Lines        2085     1910     -175     
==========================================
- Hits         1992     1829     -163     
+ Misses         93       81      -12     

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

@wshanks
Copy link
Collaborator

wshanks commented Apr 1, 2024

This looks good to me. Could you remove the 1to2 section from doc/index.rst as well?

Also, perhaps a separate discussion, but should the codecov checks be optional? It is nice to keep track of the coverage, but when it is not 100% it is hard to say whether it is good or bad that the number goes down slightly as code is added and removed. I think the ideal case is to require 100% coverage and mark code that doesn't need to be covered as "no cover".

@jagerber48
Copy link
Contributor

Mmm I'm not exactly sure how the codecov CI works. the github action that I set up submits the code to codecov.io, but then it seems like codecov.io sends data back as a github action that can pass or fail.

I'm not sure how we would "make codecov checks optional". Right now I don't think they're part of the "final results" github action, so codecov not passing doesn't block merging a PR. This seems fine to me.

I think the ideal case is to require 100% coverage and mark code that doesn't need to be covered as "no cover".

I haven't looked at the code coverage on uncertainties yet. This PR will help clean things up and then we'll be able to get a clear picture of if uncertainties is hitting 100% coverage or not. But yes, once we have that picture we might mark some lines as no cover, but I prefer to be pretty conservative when it comes to marking lines as no cover. We may need to write more tests.

@andrewgsavage
Copy link
Contributor Author

remove the 1to2 section from doc/index.rst

done

I think codecov is set up fine, we can use our own judgement for cases like this and still merge

Copy link
Collaborator

@wshanks wshanks 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 to me now!

I think you are right about codecov already being optional in the way that I meant. In any case, it is passing now 🙂

@newville
Copy link
Member

newville commented Apr 1, 2024

Looks great.
Does anyone object to merging this?

@wshanks wshanks merged commit 8421268 into lmfit:master Apr 2, 2024
16 checks passed
@wshanks
Copy link
Collaborator

wshanks commented Apr 2, 2024

I think we had enough consensus to merge here!

This was referenced Apr 2, 2024
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.

4 participants