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: primary keys need not to be integers, to serialize them is safer to use str() than int() #1806

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mapio
Copy link

@mapio mapio commented Feb 16, 2022

This patch fixes #1805.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #1806 (a4cb837) into master (c6fecdc) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #1806   +/-   ##
=======================================
  Coverage   78.29%   78.29%           
=======================================
  Files          72       72           
  Lines        8699     8699           
=======================================
  Hits         6811     6811           
  Misses       1888     1888           
Flag Coverage Δ
python 78.29% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
flask_appbuilder/views.py 66.11% <0.00%> (ø)

Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

Makes sense to me, lint is failing on CI

@dpgaspar dpgaspar changed the title Primary keys need not to be integers, to serialize them is safer to use str() than int() fix: primary keys need not to be integers, to serialize them is safer to use str() than int() Feb 24, 2022
@mapio
Copy link
Author

mapio commented Mar 4, 2022

Makes sense to me, lint is failing on CI

It actually seems that the fail on CI is due to codecov reporting that the patched lines are not covered by tests; it actually seems that all the lines in the modified file are not covered at all by any test.

I ask you to approve and merge the patch, since it will be difficult for me to add tests for all the views.

@mapio
Copy link
Author

mapio commented Mar 10, 2022

Is there reason why this isn't merged yet?

@mayurnewase
Copy link
Contributor

@mapio I think Daniel's style on merging in this repo is bit different, he will approve all finalised prs and just before cutting a release he will rebase on master and 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.

Primary keys need not to be integers, serializing them using int() causes an exception.
3 participants