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

fix DelimitedList serialize bug #319

Merged
merged 5 commits into from Nov 3, 2018

Conversation

@lee3164
Copy link

commented Nov 1, 2018

DelimitedList should use it's parents serialize results when as_string is set
Change-Id: Ic7318703fce93c64373eabf1737e2480256eb557

fix DelimitedList serialize bug
Change-Id: Ic7318703fce93c64373eabf1737e2480256eb557
@lafrech

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Thanks.

Do you think you could add a test, or rework test_delimited_list_as_string to actually test your fix?

add new DelimitedList test case
Change-Id: Ieb366cdf9e47893c8aa04dfb87fafecb68dfcace
@lee3164

This comment has been minimized.

Copy link
Author

commented Nov 1, 2018

ok, I add a new test case and test is pass

@sloria
Copy link
Member

left a comment

Looks like pre-commit step failed. Running invoke precommit should fix the issue. Feel free to amend your latest commit with the formatting change.

@@ -880,6 +881,22 @@ def test_delimited_list_as_string(web_request, parser):
assert data["ids"] == "1,2,3"


def test_delimited_list_as_string_v2(web_request, parser):
# b26793ee8021b6cfe525ac4f05570c3c68a4debd commit info

This comment has been minimized.

Copy link
@sloria

sloria Nov 1, 2018

Member

Nitpick: No need to include the commit info here.

This comment has been minimized.

Copy link
@lee3164

lee3164 Nov 2, 2018

Author

🤣,I just want to others know why I add this test case

This comment has been minimized.

Copy link
@sloria

sloria Nov 2, 2018

Member

That's what git blame is for =)

This comment has been minimized.

Copy link
@lee3164

lee3164 Nov 2, 2018

Author

ok, i will delete this line

This comment has been minimized.

Copy link
@lee3164

lee3164 Nov 2, 2018

Author

why CI cannot pass?

This comment has been minimized.

Copy link
@sloria

sloria Nov 2, 2018

Member

Let me know if you have trouble getting the command to work. I can help you work through the errors if you want.

Or, if you don't have time, I'd be glad to make the change myself.

This comment has been minimized.

Copy link
@lee3164

lee3164 Nov 2, 2018

Author

I very appreciate your kindness. I do have some troubles in this command. I do not know how to execute this command. So, could you tell the steps to work it?

This comment has been minimized.

Copy link
@sloria

sloria Nov 2, 2018

Member

In a Python 3.6 virtual environment, install the dev-requirements:

pip install -r dev-requirements.txt

Then run

invoke precommit

This will format the code using black. Then commit and push the changed files.

This comment has been minimized.

Copy link
@lee3164

lee3164 Nov 3, 2018

Author

Thank you very much, I never make a pr before

This comment has been minimized.

Copy link
@sloria

sloria Nov 3, 2018

Member

That's great! Thank you for taking the time to make the PR.

@sloria

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Also, please add yourself to AUTHORS.rst.

lee3164 added some commits Nov 2, 2018

modify authors.rst
Change-Id: Ia50d1524931b5bde8b11e983e6d6bd2fcc1a0d84
delete commit info comment
Change-Id: I49a094555157b630c3d12cef8c7206a5e7150ee2
format code
Change-Id: Ib99481fa2acb0baea56f1c4c13379b4bbb1535bf
@sloria

sloria approved these changes Nov 3, 2018

@sloria sloria merged commit 8d24186 into marshmallow-code:dev Nov 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.