Skip to content
This repository was archived by the owner on Jan 4, 2025. It is now read-only.

fix clean_params#106

Merged
GriceTurrble merged 3 commits intopython-amazon-mws:developfrom
elcolumbio:develop
Feb 25, 2020
Merged

fix clean_params#106
GriceTurrble merged 3 commits intopython-amazon-mws:developfrom
elcolumbio:develop

Conversation

@elcolumbio
Copy link
Copy Markdown
Contributor

@elcolumbio elcolumbio commented Jan 29, 2019

clean_params should not delete boolean False values. Now removes only those values which are None.

since we introduced the apis subfolder we have to declare it in the setup.py so we can pip install mws.

include apis folder in setup.py and fix clean_params
9a48010 
clean_params keeps now False values. Removes only values which are None.
@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jan 29, 2019

i am going to sleep. The if clause is still bad. If someone enters an integer 0 as value it gets removed.
Maybe add more cases or remove empty stuff in the end.

@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jan 29, 2019

or dont remove anything?

GriceTurrble
GriceTurrble reviewed Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@GriceTurrble GriceTurrble left a comment

Choose a reason for hiding this comment

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

Why was is not None not sufficient? I'd think that would be enough.

@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jan 29, 2019

agreed is not None is better.
Only for empty strings not, but do we want to remove them?

@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jan 29, 2019

i thought it would be easy, but we have to fix some more stuff.
which i think is good anyway:
in reports in line211 for example
report_types = report_types or []

but that is anyway bad, i can just remove those lines

@GriceTurrble
Copy link
Copy Markdown
Member

GriceTurrble commented Jan 29, 2019 •
edited
Loading

Yes, empty strings should be removed, as well. So v is not None and v != "". Everything else should remain.

Edit: I see no issue with that method for defaulting report_types, personally. It avoids setting a mutable default in the signature.

@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jan 29, 2019 •
edited
Loading

edited:
Line 211 is not needed since util.enumerate handles all cases.
But it's also no mistake to have it and could give people a hint that we expect a list there.

python-amazon-mws/mws/apis/reports.py

Lines 204 to 211 in 9a48010

def get_report_schedule_count(self, report_types=None):
"""
Returns a count of order report requests that are scheduled to be submitted to Amazon MWS.
Docs:
http://docs.developer.amazonservices.com/en_US/reports/Reports_GetReportScheduleCount.html
"""
report_types = report_types or []

@elcolumbio
Copy link
Copy Markdown
Contributor Author

elcolumbio commented Jun 23, 2019

this seems to be 2 helpful bug fixes. Can we merge it?

@elcolumbio elcolumbio mentioned this pull request Jul 22, 2019
Closed
@Lacrymology
Copy link
Copy Markdown

Lacrymology commented Sep 17, 2019

bump this. I was about to push a PR for the same fix

@altendky
Copy link
Copy Markdown

altendky commented Sep 17, 2019

and is not a good thing in a PR title. Might be worth making a separate PR for just the little packages thing since the discussion here seems to be about the other piece.

@Bobspadger
Copy link
Copy Markdown
Member

Bobspadger commented Sep 19, 2019

and is not a good thing in a PR title. Might be worth making a separate PR for just the little packages thing since the discussion here seems to be about the other piece.

I agree, I think this should be two PR's, one for fixing setup, one for clean_params

@elcolumbio are you able to do this ?

@elcolumbio elcolumbio mentioned this pull request Dec 20, 2019
Closed
@elcolumbio elcolumbio changed the title include apis folder in setup.py and fix clean_params fix clean_params Dec 20, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 20, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@a7886c7). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #106   +/-   ##
==========================================
  Coverage           ?   77.27%           
==========================================
  Files              ?       18           
  Lines              ?      955           
  Branches           ?       92           
==========================================
  Hits               ?      738           
  Misses             ?      212           
  Partials           ?        5
Impacted Files Coverage Δ
mws/mws.py 68.07% <100%> (ø)

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 a7886c7...f362af2. Read the comment docs.

GriceTurrble
GriceTurrble approved these changes Feb 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@GriceTurrble GriceTurrble left a comment

Choose a reason for hiding this comment

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

Long overdue. Thank you for the contribution. :)

@GriceTurrble GriceTurrble merged commit 8facbcb into python-amazon-mws:develop Feb 25, 2020
Lacrymology added a commit to Shiphero/python-amazon-mws that referenced this pull request Jul 10, 2020
@Lacrymology
Merge remote-tracking branch 'upstream/develop' into develop
66ad31f 
* upstream/develop:
  Add Feeds utility to convert dict FeedOptions to string (python-amazon-mws#181)
  Ensure correct decoding of bytes (python-amazon-mws#168)
  Compatibility fix (python-amazon-mws#124)
  Implement a couple of function for the OutboundShipments API. (python-amazon-mws#96)
  Added OutboundShipments.create_fulfillment_order (python-amazon-mws#95)
  added UpdateReportAcknowledgements (python-amazon-mws#101)
  Update feeds.py (python-amazon-mws#166)
  added easyship api support for indian marketplace (python-amazon-mws#169)
  Moving Slack link up, removing gitter chat link
  add all current marketplaces and alphabetize by country code (python-amazon-mws#155)
  Updated slack invite link
  Slack invite link (python-amazon-mws#152)
  Slack invite link
  fix clean_params (python-amazon-mws#106)
  add in NL marketplace information
  Fix flake8 warnings
  Update develop branch CI + package metadata
  include mws.apis in setup.py
  Fix bug: Remove trailing comma
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@GriceTurrble GriceTurrble GriceTurrble approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@elcolumbio @GriceTurrble @Lacrymology @altendky @Bobspadger