convert enchant resources to objects

[remicollet]

• new classes:
  • EnchantBroker
  • EnchantDict
• add OO interface
• deprecate enchant_broker_free* (use unset instead)
• deprecate ENCHANT_MYSPELL and ENCHANT_ISPELL constants

Replace pr #5528

[nikic]

I'm wondering if it really makes sense to have this constructor. It seems like there is not much benefit to allowing new EnchantDict($broker, $tag) instead of $broker->requestDict($tag), just two ways to write the same thing.

[remicollet]

requestDict will return false in case of failure, while the _construct will raise an exception, which could seems better for OO dev.

BTW, I was thinking to have a way to construct PWL, perhaps

public function __construct(EnchantBroker $broker, string $tag [,string $pwl]);

[remicollet]

Thinking a bit more:

• procedural way, use enchant_broker_request_dict and enchant_broker_request_pwl_dict
• OO way uses EnchantDict::_construct()

So Enchant::requestDict() and Enchant::requestPWL() looks like some mixed of both way.... btw, implementation is no cost (simple alias). So I prefer to keep this __construct.

[nikic]

requestDict will return false in case of failure, while the _construct will raise an exception, which could seems better for OO dev.

If this is the concern, wouldn't it be better to make requestDict() throw in the OO API?

[remicollet]

wouldn't it be better to make requestDict() throw in the OO API?

Indeed, done

[nikic]

This is unresolved. @remicollet Please remove the EnchantDict constructor.

[remicollet]

But why ?
I really think having a class without a constructor is terrible idea.
User writing OO script will of course use the $dict = new EnchantDict(...)
ANd I really don't see any issue having multiple (3) ways to write it

[remicollet]

If no more comment, plan to merge this in a few days

[remicollet]

Merged as 7db4c24 and 2c63324

[nikic]

Generally I'm unhappy with this merge. Converting enchant to use objects? Okay, that only needs a quick technical review. Adding a new OO API? This requires a more careful design review (and possibly notice to the internals mailing list) to make sure that the new OO API is actually good, and not just a copy of an unfortunate procedural API. I don't think anyone has done this kind of review here.

Apart from the questionable EnchantDict constructor (it now even has a required $tag parameter that just gets ignored if you pass $pwl, huh?) there are also other weird decisions here. For example, what's up with the naming of EnchantBroker::isDict()? The enchant_broker_dict_exists() name in the original API makes more sense than that. dictExists() or hasDict() would both work, isDict() doesn't work.

[remicollet]

OK, everything reverted, let find someone else take care of this extension

[nikic]

Uh, this is not the outcome I was looking for... To be clear, I appreciate the work you did here, and that you went through the effort to actually add a new OO API, instead of just leaving it as opaque objects. I also think that the API is mostly fine as is, but I would have liked to iron out some of the details more, and try to find a consensus on the issue we disagreed on (constructor). This was not meant as "this change is bad", but as "it was a bit too early to merge this".

[remicollet]

Sorry, but it seems I'm too tired.

I think this extension needs to be properly maintained, as the only modern dictionary extension (as a/pspell seems deprecated in modern distro), and was thinking I will be able to take this work.

I should probably have followed a better workflow (internals, rfc...)
This is a personal failure

But I don't have the strength to try (again) to reach a consensus.
I probably accept too much work for my personal resources.
So really better to find someone else to maintain this one.

Feel free to pick my work, and change what you want