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 invalid panel dx/dy offsets #294

Merged
merged 3 commits into from Jul 4, 2018

Conversation

Projects
None yet
3 participants
@carandraug
Contributor

carandraug commented May 21, 2018

After parsing the JSON string, check if it's valid and fix it where we can. Fixes issue #292

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore May 21, 2018

Member

I think you'll need to call your fix on each panel, not on the figure_json, since dx and dy are attributes of each panel.
Add your fix after this line:

for i, panel in enumerate(panels_json):
    # fix panel json here...

Or you could iterate through all the panels in your _fix_figure_json() and fix them all at once at the start.

I think you'll need to call your fix on each panel, not on the figure_json, since dx and dy are attributes of each panel.
Add your fix after this line:

for i, panel in enumerate(panels_json):
    # fix panel json here...

Or you could iterate through all the panels in your _fix_figure_json() and fix them all at once at the start.

This comment has been minimized.

Show comment
Hide comment
@carandraug

carandraug May 21, 2018

Contributor

Ups. You're right. I fixed it and this time actually checked the fix.

Contributor

carandraug replied May 21, 2018

Ups. You're right. I fixed it and this time actually checked the fix.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore May 21, 2018

Member

I think that negative offsets are valid. Only need to handle None.

I think that negative offsets are valid. Only need to handle None.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore May 21, 2018

Member

Best to continue using the DEFAULT_OFFSET parameter here as suggested by @jburel in #257 (comment)

Member

will-moore commented on omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py in 9e70bd9 May 21, 2018

Best to continue using the DEFAULT_OFFSET parameter here as suggested by @jburel in #257 (comment)

This comment has been minimized.

Show comment
Hide comment
@carandraug

carandraug May 21, 2018

Contributor

@jburel comment was about having the default logic in a single place (that variable is used in 4 places). Because this change reduces the use of that logic to a single place, using a variable to reduce its duplication is no longer necessary. Unless DEFAULT_OFFSET is meant to be configurable by a user?

Contributor

carandraug replied May 21, 2018

@jburel comment was about having the default logic in a single place (that variable is used in 4 places). Because this change reduces the use of that logic to a single place, using a variable to reduce its duplication is no longer necessary. Unless DEFAULT_OFFSET is meant to be configurable by a user?

This comment has been minimized.

Show comment
Hide comment
@carandraug

carandraug May 21, 2018

Contributor

I put the variable back nonetheless.

Contributor

carandraug replied May 21, 2018

I put the variable back nonetheless.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore May 21, 2018

Member

I've not tested yet but looks good.
We will be releasing figure 4.0.0 very soon so this won't go into that release I'm afraid.
I just noticed travis failed:

./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:651:9: E266 too many leading '#' for block comment
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:652:9: E266 too many leading '#' for block comment
Member

will-moore commented May 21, 2018

I've not tested yet but looks good.
We will be releasing figure 4.0.0 very soon so this won't go into that release I'm afraid.
I just noticed travis failed:

./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:651:9: E266 too many leading '#' for block comment
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:652:9: E266 too many leading '#' for block comment
@carandraug

This comment has been minimized.

Show comment
Hide comment
@carandraug

carandraug May 21, 2018

Contributor

I've not tested yet but looks good.
We will be releasing figure 4.0.0 very soon so this won't go into that release I'm afraid.

No problem. We already are using a modified version of the script that skips the annotation after the build.

I just noticed travis failed:

I have changed the comment style to follow pep 8

Contributor

carandraug commented May 21, 2018

I've not tested yet but looks good.
We will be releasing figure 4.0.0 very soon so this won't go into that release I'm afraid.

No problem. We already are using a modified version of the script that skips the annotation after the build.

I just noticed travis failed:

I have changed the comment style to follow pep 8

@sbesson

sbesson approved these changes May 21, 2018 edited

This PR tries to address the bug described in #292 which had already been raised in #189 and tentatively fixed in #257 .

The proposed changes should handle the scenario where the dx key or the dy key exists but has a null value and set its value to DEFAULT_OFFSET in the context of the export script.

The change reverts the usage of dict.key(key, default) introduced in 77fd869. This logic would still be necessary if the keys could be missing under certain conditions as the script would raise a KeyError. Since the original PR (#257) was attempting to fix the same bug as here, it is possible the code path defining a default dictionary value in the getter was never used in practice. Leaving @will-moore to comment on this point.

Overall, 👍 for getting this into a 4.0.1.

@will-moore will-moore merged commit 7ec1718 into ome:master Jul 4, 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