Initialize headers from config in reset_base_headers()#707
Initialize headers from config in reset_base_headers()#707glensc wants to merge 6 commits intopushingkarmaorg:masterfrom
Conversation
glensc
force-pushed
the
headers-later-init
branch
from
59d6862 to
544ae2f
Compare
|
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:
|
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?
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.
|
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. |
glensc
force-pushed
the
headers-later-init
branch
from
1c5505d to
89f8bff
Compare
2 participants
WIP: #706