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

Bugs web sprint2 2 #450

Merged
merged 13 commits into from Nov 2, 2012
Merged

Bugs web sprint2 2 #450

merged 13 commits into from Nov 2, 2012

Conversation

will-moore
Copy link
Member

This was previously #447 but rebased to fix merge conflicts.

@joshmoore
Copy link
Member

NB: I wonder if we almost need to add methods to import these which does things in the right order. Something like:

Image = ImportPILImage() ImageDraw = ImportImageDraw()

I tried to keep these synchronized with the idiom:

@connection-rebase ~/git/components/tools/OmeroPy $ git grep -B 2 -A 2 ticket:2597 src/flim-omero.py- src/flim-omero.py-try: src/flim-omero.py: from PIL import Image, ImageDraw # see ticket:2597 src/flim-omero.py-except ImportError: src/flim-omero.py: import Image, ImageDraw # see ticket:2597

so that the following query should return nothing:

@connection-rebase ~/git/components/tools/OmeroPy $ git grep "import Image" | grep -v ticket:2597 | grep -vE "(ImageI|ImageAnnotationLinkI|ImageWrapper)" src/omero/gateway/__init__.py: import Image, ImageDraw, ImageFont src/omero/gateway/__init__.py: from PIL import Image, ImageDraw, ImageFont src/omero/util/populate_metadata.py:from omero.grid import ImageColumn, LongColumn, PlateColumn, StringColumn, \ src/omero/util/populate_roi.py:from omero.grid import ImageColumn, WellColumn, RoiColumn, LongColumn, DoubleColumn src/omero/util/script_utils.py: from PIL import Image src/omero/util/script_utils.py: import Image src/omero/util/uploadMask.py:from OmeroPopo import ImageData test/integration/bigImages.py: from PIL import Image

but likely these other locations were modified after my ticket:2597 changes. (Here I'm grepping in a different branch)

@manics
Copy link
Member

manics commented Oct 31, 2012

#9586 is still a problem (open script, change datatype, ID's aren't cleared).

@will-moore
Copy link
Member Author

Hmmm - I can see the logic behind ImportPILImage() etc, but then you've still got to import ImportPILImage from somewhere, which is kinda ugly. And then there's still no guarantee that anyone is going to use it (and not do their own PIL import).

@joshmoore
Copy link
Member

@will-moore, it's a fair point, which is also, I guess, why I didn't go with anything like that. Perhaps just a pylint-style check to make sure all imports are done in the proper style would be good. And barring that, teach the team what to look for, so everyone reviews the PRs.

This is not the bug that was originally reported in #9804 (Safari working fine now) but was
noticed as part of that testing.
@will-moore
Copy link
Member Author

Previous commit fixes Shape previews for shapes with no Z / T specified and Polygons with only a single point.

@@ -43,10 +43,10 @@
logger = logging.getLogger(__name__)

try:
import Image, ImageDraw, ImageFont
from PIL import Image, ImageDraw, ImageFont
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth including the comment with the ticket number so that the grep command I listed will return nothing?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, but #2597 makes no mention of the need to try import 'from PIL' first. So that ticket will need to be updated with the info (and link) to #9790.

@bpindelski
Copy link

Merges fine with dev_4_4 locally (for testing if bin/omero web start succeeds).
70a7ddf looks ok but doesn't display the build number (only "OMERO.web .").

git grep "import Image" | grep -v ticket:2597 | grep -vE "(ImageI|ImageAnnotationLinkI|ImageWrapper|ImageColumn|ImageData)" returns nothing.

After correcting the lacking build number in the /webstart landing page, it's ready to merge.

joshmoore added a commit that referenced this pull request Nov 2, 2012
@joshmoore joshmoore merged commit 6879e42 into ome:dev_4_4 Nov 2, 2012
@will-moore will-moore mentioned this pull request Nov 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants