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

Convert factory functions to proper python classes #43

Open
djhoese opened this issue Nov 25, 2018 · 4 comments
Open

Convert factory functions to proper python classes #43

djhoese opened this issue Nov 25, 2018 · 4 comments
Labels
help wanted Primary maintainers may not have time to resolve this priority: medium

Comments

@djhoese
Copy link
Member

djhoese commented Nov 25, 2018

Aggdraw seems to define all of its class __new__ methods as functions in the aggdraw module. It then creates a new object by creating a new type. This is an issue because this means classes like aggdraw.Draw aren't actually classes, they are functions, and that a user has no access to the methods without creating an instance of the object. Even worse you can't use sphinx autodoc functionality to document the classes because they don't technically exist until an instance is made.

I don't think this will be extremely hard to implement, but it isn't a top priority for me. Additionally, it has a high chance of breaking something.

@djhoese djhoese added help wanted Primary maintainers may not have time to resolve this priority: medium labels Nov 25, 2018
@djhoese djhoese changed the title Convert factor functions to proper classes Convert factory functions to proper python classes Nov 25, 2018
@karimbahgat
Copy link

karimbahgat commented Feb 4, 2019

Wouldn't mind helping on this, as it's just more stable and also affects clues from autocomplete, plus I suspect the missing parts from https://aggdraw.readthedocs.io/en/latest/.
But this would all have to be done in aggdraw.cxx, ie in C++, correct?

@ismaelharunid
Copy link

I have the same interest. I thought I raised an issue but I don't see it here. Might be because of my crappy internet. Anyway I would love to see the classes become actual classes but I only started using aggdraw a week ago so not sure how much help or damage I can be. But I'll start going through the code and investigate what might be required

@ismaelharunid
Copy link

So what happened? This implementing as classes is moved to another task, or forgotten?

@djhoese
Copy link
Member Author

djhoese commented Jun 29, 2020

You did make an issue in #68 where I commented with my concerns and then closed it as a duplicate of #45. In that comment I pointed out how there are some current issues with current master branch and how it doesn't match the exact output of the currently released version of aggdraw. While the cython (#45) could be worked on while the current master branch implementation has these inconsistency fixes applied in parallel, there isn't really enough developer time to do this. It sounds like you are willing to look at doing the cython stuff. If so, you can continue commenting on #45 with questions and make a pull request as soon as possible and we can review your code/updates together.

As far as "what happened", this project is completely volunteer work. For the pytroll group, aggdraw is a utility library used by a utility library. The libraries that use these libraries are also all volunteer work. Aggdraw is "good enough" and works for us so major refactors like this will always/likely fall to outside contributors as we won't have time to do this work "for fun".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Primary maintainers may not have time to resolve this priority: medium
Projects
None yet
Development

No branches or pull requests

3 participants