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

Known problems with solutions in README file modified with a fix for … #148

Conversation

xalien10
Copy link

@xalien10 xalien10 commented Sep 1, 2021

Known problems with solutions in README file modified with a fix for formdata update of nested fields via PUT or PATCH request

@ir4y
Copy link
Member

ir4y commented Sep 2, 2021

Thank you for your contribution @xalien10
There are two small notes.

  1. Could you please leave the only minimum necessary set of fields in the example? There are a lot of fields that don't need to represent the issue and it confuses.
  2. Could you please provide an example of a working serializer or at least some part of it? You describe the technique, it is cool and should be in the README, but also it would be nice to provide more context where this code and be user, what serializer's controller's methods need to be override?

@xalien10
Copy link
Author

xalien10 commented Sep 2, 2021

@ir4y I'll do that. Thank you for your feedback

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021

Codecov Report

Merging #148 (ba5a629) into master (c6c47e4) will not change coverage.
The diff coverage is n/a.

❗ Current head ba5a629 differs from pull request most recent head 10b4e02. Consider uploading reports for the commit 10b4e02 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files           3        3           
  Lines         220      220           
=======================================
  Hits          216      216           
  Misses          4        4           

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 c6c47e4...10b4e02. Read the comment docs.

…formdata update of nested fields via PUT or PATCH request

Known problems with solutions in README file modified with a fix for formdata update of nested fields via PUT or PATCH request

Known problems with solutions in README file modified with a fix for formdata update of nested fields via PUT or PATCH request

Known problems with solutions section in README file modified to provide solution for formdata with nested fields
@xalien10 xalien10 force-pushed the update-problem-with-nested-fields-and-formdata branch from ba5a629 to 10b4e02 Compare September 2, 2021 14:35
@xalien10
Copy link
Author

xalien10 commented Sep 2, 2021

@ir4y I've updated the PR with more concrete example. Please take a look when you have some time for it.

@ir4y
Copy link
Member

ir4y commented Sep 15, 2021

@xalien10
Sorry for the long reply. It seems that everything is almost done.
May I kindly ask you to add a test that demonstrates this behavior?
I think it will provide an even better experience than just a code snippet.

@ir4y
Copy link
Member

ir4y commented Sep 23, 2021

Ok, I am merging the documentation. If somebody face some issue with examples we will revisit the topic about tests.
Thank you @xalien10

@ir4y ir4y merged commit b3fff32 into beda-software:master Sep 23, 2021
@xalien10
Copy link
Author

@ir4y I'm extremely sorry that I couldn't reply you back. Currently, I'm loaded heavily so couldn't write appropriate tests for the solution. If I can manage some time I'll definitely make a PR on that including the tests you asked for.

@auvipy
Copy link

auvipy commented Jul 17, 2022

@ir4y I'm extremely sorry that I couldn't reply you back. Currently, I'm loaded heavily so couldn't write appropriate tests for the solution. If I can manage some time I'll definitely make a PR on that including the tests you asked for.

good job bro

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.

4 participants