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

Allow the adding of headers rather than just overwriting all headers #958

Closed

Conversation

pradcliffe-ns
Copy link
Contributor

@pradcliffe-ns pradcliffe-ns commented Aug 7, 2020

In the current update of the options dict you either overwrite all default headers or none. This change allows the addition of headers or changing of existing headers without removing other default headers. That way the current headers entries in DEFAULT_OPTIONS will not need to be duplicated in code that uses the module and if the default headers set is updated in the future it will be used as expected.

@studioj
Copy link
Collaborator

studioj commented May 24, 2021

@pradcliffe-ns please merge in the changes from master, solve the conflicts and maybe explain the added functionality improvement in the PR itself (some code sample would be nice)

@studioj studioj reopened this May 24, 2021
@pradcliffe-ns
Copy link
Contributor Author

pradcliffe-ns commented May 24, 2021

@pradcliffe-ns please merge in the changes from master, solve the conflicts and maybe explain the added functionality improvement in the PR itself (some code sample would be nice)

@studioj I filed this 9 months ago and was trying to be helpful in adding functionality that was useful as explained above. I'm working on different projects now and I'm not sure I have time to add to this. Going back to try and produce example code and work out where in your project those belong is not trivial.

It's a pretty simple change that allows you to add headers without overwriting them all as I explained above, anything in headers within DEFAULT_OPTIONS -

DEFAULT_OPTIONS = {
- that you need to change or add extra headers requires overwriting the whole set rather than just making one change. If you don't want that functionality then close the PR.

@studioj
Copy link
Collaborator

studioj commented May 24, 2021

I believe it would be smart to add a test verifying this behavior. But that's my QA person opinion :) in the end up to @ssbarnea :-) i just try to offload some work of of him.

@pradcliffe-ns
atm CI tests are failing because of black in client.py as you can see here https://github.com/pycontribs/jira/pull/958/checks?check_run_id=2657872717#step:8:47

@pradcliffe-ns
Copy link
Contributor Author

I have no idea what "because of black" means.

@studioj
Copy link
Collaborator

studioj commented May 24, 2021

I have no idea what "because of black" means.

@pradcliffe-ns this repo uses the uncompromising code formatter "black" https://pypi.org/project/black/ which is rapidly becoming a standard as code formatting goes.

if you don't know it yet, it's a very interesting project.
=> have a look at their read the docs https://black.readthedocs.io/en/stable/

how to fix your code ...

pip install black
black client.py
then save commit and push

@ssbarnea
Copy link
Member

Please assure that at least running tox -e lint is passing before uploading a change. That would run all the linters, including black. its output should be explanatory. If not fixed, we wil lclose the PR.

@adehad adehad mentioned this pull request Jun 12, 2021
@adehad
Copy link
Contributor

adehad commented Jun 26, 2021

Refer to #1085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants