Skip to content
This repository was archived by the owner on Apr 12, 2023. It is now read-only.

Conversation

@Scondo
Copy link

@Scondo Scondo commented Aug 16, 2013

Implementing filter types other than '0'.
This can be nice to achieve better compression.
Right now only one filter per file applied.

Also core of applying filter and undo filter switched to 'augmented' py to make python development easier and keeping cython speedup.

This is my first experience to contribute, sorry if something wrong.

Scondo added 12 commits July 23, 2013 13:33
Pro: same code for cython and python
Contra: manual compiling reqired

pyx left for checks and references
Seems to be old implementation of pngfilters "undo"...
This can be useful for picture viewer or something like this.
And few bugs around
For speedup by cython
C-result of Cython included to allow compilation during install
Speedup about 10% in unittests (python 2.7, compiled filters)
@drj11
Copy link
Owner

drj11 commented Aug 22, 2013

In principle this is great. But you've added another python file. I'd like to keep all of the code for "import png" in one file. See this comment on the original Cython pull request: #9 (comment)

@Scondo
Copy link
Author

Scondo commented Aug 22, 2013

I think two files is better than two copies of same code.
Note that current Cython extensions still have another file which imported during "import png".
Line 180 now:

try:
    import pyximport
    pyximport.install()
    import cpngfilters as pngfilters

Code inside png.py is just a fallback for case when import of cpngfilters.pyx is failed. And different code in pyx and fallback could lead to situation when some code would be changed in one, but not in other. After such thing "png.py" would be ok with cython, but crash without (or reverse).
To avoid this situation I switched from pyx (which already was a separate file) to py+pxd using same code for compilation and fallback.

@drj11
Copy link
Owner

drj11 commented Aug 23, 2013

I think I made to fail to make myself clear:

When using a plain Python installation (without Cython), I require that all the relevant code is in one file: png.py.

I don't really care how or what you Cython users do, as long as it doesn't change that.

@Scondo
Copy link
Author

Scondo commented Aug 24, 2013

Well, I can move code to png.py and write some script to save that into separate file before making ".c" file.
But can you give me more explanation why this strange "unimport" trick reqired?

I just don't feel difference between single file or two files after import one in another.

@Scondo
Copy link
Author

Scondo commented Aug 29, 2013

I've been thinking about your requirement all these days...
Would be ok for you if I'll copy (may be with script) filters code as class into png.py? Like super-safe fallback?

So pngfilters.py will produce ".c" code and class in png.py. Import try get binary version, if fails - pngfilters.py, if fails - we use except to use class.

Make pngfilters work even when import of pngfilters.py failed.
@Scondo
Copy link
Author

Scondo commented Sep 7, 2013

Check super-safe pngfilters. And please, write your reason to this limitation for future developers.
Until someone doesn't write thing that couldn't be tricked such way.

@drj11
Copy link
Owner

drj11 commented Sep 10, 2013

I'll have a look on Friday.

@drj11
Copy link
Owner

drj11 commented Sep 10, 2013

Where would you want the reason documented? How about "It's intended that you can copy png.py into your application and distribute it" from the README ?

@Scondo
Copy link
Author

Scondo commented Sep 10, 2013

Hm... may be after 'except' keyword... so when developer will see this trick he would know it's purpose.

And... Is there any other reason? If not - ok, I'll write this remarks in comments before you pull.

*But may be it's still worth to switch into package sometimes later.

@drj11
Copy link
Owner

drj11 commented Oct 4, 2013

Explain what filters_embed.py does.

Copy link
Owner

Choose a reason for hiding this comment

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

I feel ill. pull request rejected.

Copy link
Owner

Choose a reason for hiding this comment

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

decorators? You've just made this file FAIL TO COMPILE for Python 2.2 and Python 2.3. Not good.

Copy link
Author

Choose a reason for hiding this comment

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

This (and also missed error for except) because all this part was written as just proof-of-concept while I'm trying to understand you requirement about 'all code in one file' .

Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain why you expect me to merge something that you admit is a proof-of-concept?

Copy link
Author

Choose a reason for hiding this comment

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

I do not expect you merge this. I expect you estimate this. So I add code to pull-request as part of dialog.

Copy link
Owner

Choose a reason for hiding this comment

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

Duly noted.

@drj11
Copy link
Owner

drj11 commented Oct 4, 2013

I've now had a longer look. You've made png.py a compiled file. That sort of thing will never be accepted as a PR.

png.py must be the primary source for editing, and just the single file png.py must work on unmodified versions of Python 2.2 through to 2.7.

@drj11 drj11 closed this Oct 4, 2013
@drj11
Copy link
Owner

drj11 commented Oct 4, 2013

Get on the mailing list and start a discussion about what you're trying to achieve.

@Scondo
Copy link
Author

Scondo commented Oct 4, 2013

"png.py must be the primary source for editing, "
That what I'd asked about. Why we must edit only one file?

My English may be bad, but I still have no clue why. If I had this reason I wouldn't do this crazy tricks with super-safe import.

@drj11
Copy link
Owner

drj11 commented Oct 5, 2013

One File Policy:

a) one file is good for installation and embedding;
b) making png.py a compiled file smells bad.

This PR is not the place for a discussion of PyPNG's One File Policy. Any more discussion should be on a separate issue or on the mailing list.

If you want to discuss how PyPNG should play nicely with people using Cython, get on the mailing list.

Cython will never be an important audience, but I'm would like to discuss how to integrate with Cython users.

Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't do what you think. Although bytearray doesn't appear until Python 2.6, python 2.x (x<6) is quite happy to define a function with a reference to the global bytearray. So the body of the except: never gets run, even on Python 2.5 and below.

Better:

try:
    bytearray
except:
    def bytearray( blah blah blah )...

Even better: Don't write this sort of stuff until you've actually tested it on earlier Python versions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants