-
Notifications
You must be signed in to change notification settings - Fork 55
Description
Currently, Sequence implements the list of rules to be matched against. And OrderedChoice inherits from Sequence. But OrderedChoice matches only one of the rules that it contains. So This class violates one of the basic principles of OOP which is that any descendant should be compatible with its parent. But in this case their behavior is absolutely different.
Optional has a similar problem. It inherits from Repetition but has nothing to be repeated. Repetition's sep and eolterm attributes are useless for the Optional class.
Some other problems with abstract classes were solved in the #131 pull request. But I haven't reviewed the whole parser, so similar issues might still be present in the library.
Also, there are some problems with the dependencies between classes. The parser rules shouldn't depend on the parser which is dependent on them. As I think, config parameter (like skipws) should be moved to a separate config class, and the state attributes (like position) should be moved to the state class (which appeared in the #131 pull request). Passing the config and the state to the rules would eliminate cross-dependencies.
All of these problems make the source code hard to understand. Possible contributors might not want to make changes to the source code unless it is cleaned up.
Although I must admit that fixing architecture problems would break backward compatibility,
Updated on 2025-07-28:
Some of the class methods are called exclusively by checking their class type. The compile() method of RegExMatch is called this way. Such methods should be declared in the base class so that any inheritor could override them. So there would be no need in extra checks in the code, parent methods would do nothing. Particularly, I think the resolve() method from #131 could be reused for this purpose for the initialization code.