Skip to content

Make MultiDict mutable#1165

Merged
stealthycoin merged 1 commit intoaws:masterfrom
stealthycoin:make-query-params-mutable
Jul 9, 2019
Merged

Make MultiDict mutable#1165
stealthycoin merged 1 commit intoaws:masterfrom
stealthycoin:make-query-params-mutable

Conversation

@stealthycoin
Copy link
Contributor

fixes #1158

Make MultiDict mutable again. The initial pull request made it readonly which is not backwards compatible with the raw dict it used to be.

@stealthycoin stealthycoin requested review from jamesls and kyleknap July 8, 2019 21:21
@codecov-io
Copy link

Codecov Report

Merging #1165 into master will decrease coverage by 0.05%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
- Coverage   95.61%   95.56%   -0.06%     
==========================================
  Files          27       27              
  Lines        4648     4663      +15     
  Branches      588      591       +3     
==========================================
+ Hits         4444     4456      +12     
- Misses        134      136       +2     
- Partials       70       71       +1
Impacted Files Coverage Δ
chalice/app.py 97.02% <94.73%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4853c6c...0de7074. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jul 8, 2019

Codecov Report

Merging #1165 into master will decrease coverage by 0.05%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
- Coverage   95.61%   95.55%   -0.06%     
==========================================
  Files          27       27              
  Lines        4648     4657       +9     
  Branches      588      588              
==========================================
+ Hits         4444     4450       +6     
- Misses        134      136       +2     
- Partials       70       71       +1
Impacted Files Coverage Δ
chalice/app.py 97% <92.3%> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4853c6c...e3b53f7. Read the comment docs.

chalice/app.py Outdated

def __getitem__(self, k):
values_list = self._dict[k]
def add(self, k, v):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this method for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah a stub I forgot. Initially it was going to be https://werkzeug.palletsprojects.com/en/0.15.x/datastructures/#werkzeug.datastructures.MultiDict.add

Since there is no other way to add a new value to an internal list, other than getting it, appending and resetting. Initially I was going to use it during __init__ when creating the internal structure from a list of tuples. However it was awkward since the internal map hasn't been initialized so I opted to write it all out there in the __init__ method so there is no real reason to have it, since it didn't exist before. I just forgot to remove the stub.

@stealthycoin stealthycoin force-pushed the make-query-params-mutable branch from 046f987 to e32d71e Compare July 9, 2019 16:08
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 🚢 Make sure to add a changelog as well.

pop_result = d.pop(key)
assert popped == pop_result

print(d._dict)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover print statement.

@stealthycoin stealthycoin force-pushed the make-query-params-mutable branch from 56f0bb1 to e3b53f7 Compare July 9, 2019 19:30
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to merge

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.

query_params now returns a chalice.app.MultiDict object

4 participants