Uh oh!
There was an error while loading. Please reload this page.
Initialize headers from config in reset_base_headers() - #707
Conversation
Uh oh!
There was an error while loading. Please reload this page.
glensc
commented
Mar 22, 2021
OK, here's some idea that could work. Don't mind the PR title and body, will update them once the PR comes useful. So, WDYT, is this acceptable? |
| 'X-Plex-Device': plexapi.X_PLEX_DEVICE, | ||
| 'X-Plex-Device-Name': plexapi.X_PLEX_DEVICE_NAME, | ||
| 'X-Plex-Client-Identifier': plexapi.X_PLEX_IDENTIFIER, | ||
| 'X-Plex-Platform': CONFIG.get('header.platorm', CONFIG.get('header.platform')), |
There was a problem hiding this comment.
Maybe it's about time to drop support for the 'header.platorm' typo?
There was a problem hiding this comment.
it was fixed in 2018, matching 3.2.0 version:
glensc
commented
Apr 4, 2021
How about this approach? If needed, I could extract to smaller changes. |
I still don’t see the point. IMO you need to add a example on why this is better. Fix your flake errors, run the test suit locally, including the tests for the client and squash the commits. I don’t like the name reset_default_headers as isn’t anymore, is the headers is in config file (it was a bad name in the first place) Why is the headers a local variable the class? Why not use it directly? |
| return dict(config) | ||
| def _defaults(self): | ||
| from uuid import getnode |
| from platform import uname | ||
| from plexapi import PROJECT, VERSION | ||
| platform_name, device_name, platform_version = uname()[0:3] |
There was a problem hiding this comment.
Explain fix how? fix what? what is wrong with the current code?
There was a problem hiding this comment.
why is the current slicing wrong. how is the correct way?
glensc
commented
Apr 5, 2021
I don't see point fixing tests until it's certain this is worth finishing! That's why I asked about the changes, not the style! Also, do note the PR is in a Draft state. |
I used an existing name, please suggest a better name not only complain it's bad.
|
glensc
commented
Apr 5, 2021
I think I explained it in an issue this PR is referring to (#706). to make the headers instance-based, not global. so the since Config object may be modified runtime (after ps: since Github has no threading in comment mode, submit a review as code comments, so can reply in a thread and resolve disucssions. |
1c5505d to
89f8bffCompare
WIP: #706