convert enchant resources to objects#5533
Conversation
ext/enchant/enchant.stub.php
Outdated
|
|
||
| final class EnchantDict | ||
| { | ||
| public function __construct(EnchantBroker $broker, string $tag) {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]);
There was a problem hiding this comment.
Thinking a bit more:
- procedural way, use
enchant_broker_request_dictandenchant_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.
There was a problem hiding this comment.
requestDictwill returnfalsein case of failure, while the_constructwill 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?
There was a problem hiding this comment.
wouldn't it be better to make requestDict() throw in the OO API?
Indeed, done
There was a problem hiding this comment.
This is unresolved. @remicollet Please remove the EnchantDict constructor.
There was a problem hiding this comment.
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
c05c7a2 to
2e6be49
Compare
|
If no more comment, plan to merge this in a few days |
- EnchantBroker - EnchantDict add OO interface deprecate enchant_broker_free* (use unset instead) deprecate ENCHANT_MYSPELL and ENCHANT_ISPELL constants
b6c3239 to
f1d23eb
Compare
|
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 |
|
OK, everything reverted, let find someone else take care of this extension |
|
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". |
|
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...) But I don't have the strength to try (again) to reach a consensus. Feel free to pick my work, and change what you want |
Replace pr #5528