Replies: 4 comments 3 replies
-
Converting this to a discussion... |
Beta Was this translation helpful? Give feedback.
-
@hackermd, thanks for your thoughts on this. I have a few quick thoughts that I will outline, but will also keep thinking on it further. I'm not exactly clear on how this necessarily messes with other libraries. Since Python imports are really done once and point to the same instance, another library can Another thought is that we recently have been migrating more and more to a My last thought is a bit of an explanation - global settings were mainly done to help provide the user flexibility in dealing with invalid DICOM or unusual situations. Adding a large number of arguments to already complicated call signatures seemed a bad tradeoff. However, I think migrating to the |
Beta Was this translation helpful? Give feedback.
-
@hackermd, you've made good points. As I noted, we have been trying to improve this by (as a start) moving new settings (and some old ones, more to come) into the Settings class and making that the recommended path forward. We will have to deal with the existing global config parameters for many years to come - removing those is very much a breaking change. I don't think I would advocate removing those even in pydicom 3.0, but perhaps in 4.0. I could address more details of your comments, but I'm not sure it is really helpful, perhaps we need to get to code examples to hammer it all out. I'm happy to think about this further and come up with a PR we can start with - likely along the lines of a |
Beta Was this translation helpful? Give feedback.
-
Just chiming in here to say that I agree with @hackermd that the global configuration options are problematic, and creates some awkward issues for us when developing highdicom: we would love to make use of the great new validation functionality introduced in the 2.3.0 to ensure that the attributes we set during our routines have valid values, but this could be disrupted by the user or another library based on pydicom changing the global config value at any time. Thanks for the thoughtful response. I agree that some further parameters (in a Happy to be involved in future discussions and help with PRs when an overal direction is established. |
Beta Was this translation helpful? Give feedback.
-
I noticed the trend that more and more behavior of the library gets specified via global configuration. This approach is very problematic in my opinion, because it makes it hard for other libraries that build on top of pydicom to predict what the ultimate behavior will be. When a user or an application changes the global configuration, the functionality of the other library may break in unexpected ways. We ran into this issue numerous times with highdicom.
Sometimes, the global configuration can be explicitly overridden via a local variable:
pydicom/pydicom/dataelem.py
Lines 193 to 194 in 0fa18d2
pydicom/pydicom/valuerep.py
Lines 847 to 848 in 1a49318
However, most times the global configuration gets applied directly without the ability of another library to control behavior:
pydicom/pydicom/dataelem.py
Line 843 in 0fa18d2
pydicom/pydicom/dataelem.py
Line 867 in 0fa18d2
pydicom/pydicom/dataelem.py
Line 825 in 0fa18d2
pydicom/pydicom/dataelem.py
Line 543 in 0fa18d2
pydicom/pydicom/filereader.py
Line 239 in 85d0948
While it may be tempting to give users the ability to configure behavior globally without having to change the public API, it is bad practice for a library in my opinion. Global configuration is evil! Please reconsider the approach.
Beta Was this translation helpful? Give feedback.
All reactions