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

Replace the warning log with a warnings.warn instead. #138

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

janwijbrand
Copy link

As a result the warning is only issued once by default. A developer can choose to configure what warnings are filtered how.

Rationale: When using fhir.resources from an application framework such as fastapi, that expects to be able to use the Pydantic APIs for dict() and json() etc. the log will be "spammed" with warnings. The developer that uses the framework though has very limited control over this short of re configuring the log levels.

@janwijbrand
Copy link
Author

janwijbrand commented Sep 28, 2023

Address #90 with an alternative approach. See #140

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (1fd6ea4) 99.97% compared to head (6b197cd) 99.97%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #138   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files         302      302           
  Lines       60584    60584           
=======================================
  Hits        60566    60566           
  Misses         18       18           

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

@nazrulworld
Copy link
Owner

Thanks for your PR, it seems good but can you check Travis build for python 3.10

@janwijbrand
Copy link
Author

Thanks for your PR, it seems good but can you check Travis build for python 3.10

I did have a look, but I'm a little bit at a loss here...

I'm tried run mypy locally: I get a ton of errors that I don't see on travis. Yet, the error on travis I don't see locally. The error seems to occur or at least is related to numpy. AFAICS I don't think fhir.resources itself depends on numpy. I think numpy is a (test?) dependency for orjson.

Perhaps you can trigger a travis build of the main branch manually (I don't seem to have that permission of travis) to see if the error also occurs there?

If you have any other idea, let me know!

@nazrulworld
Copy link
Owner

nazrulworld commented Oct 2, 2023

Thanks for your PR, it seems good but can you check Travis build for python 3.10

I did have a look, but I'm a little bit at a loss here...

I'm tried run mypy locally: I get a ton of errors that I don't see on travis. Yet, the error on travis I don't see locally. The error seems to occur or at least is related to numpy. AFAICS I don't think fhir.resources itself depends on numpy. I think numpy is a (test?) dependency for orjson.

Perhaps you can trigger a travis build of the main branch manually (I don't seem to have that permission of travis) to see if the error also occurs there?

If you have any other idea, let me know!

I am not 100% sure but the most provably, because of mypy version in your local, you are seeing too many errors but not in travis. for example if look at inside setup.py //"mypy" + (PY_VERSION_11_OR_LATER and ">=0.991" or "==0.812")//

I can also look that linting error, but unfortunately I am too busy right now :)

@janwijbrand
Copy link
Author

janwijbrand commented Oct 2, 2023

I am not 100% sure but the most provably, because of mypy version in your local, you are seeing too many errors but not in travis. for example if look at inside setup.py //"mypy" + (PY_VERSION_11_OR_LATER and ">=0.991" or "==0.812")//

I can also look that linting error, but unfortunately I am too busy right now :)

I'll try to dig a bit further as well.

In the meantime I tried removing the warn() call from my PR to see if the Travis outcome for the mypy check was different. It wasnt. The Travis outcome as before, so it is not my code per sé that makes the mypy fail. I added the warn() code back again.

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