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

Upgrade regexp-parser dependency #1361

Merged
merged 2 commits into from
Jan 6, 2023
Merged

Upgrade regexp-parser dependency #1361

merged 2 commits into from
Jan 6, 2023

Conversation

mbj
Copy link
Owner

@mbj mbj commented Jan 6, 2023

  • This should be more future proof, given maintenance of the AST mapper is not required anymore.
  • The Parser::Expression class does not have a "nice" interface so a new mutator hierarchy is needed.
  • Overall it should be easier to maintain.

* This should be more future proof, given maintenance of the AST mapper
  is not required anymore.
* The `Parser::Expression` class does not have a "nice" interface so
  a new mutator hierarchy is needed.
* Overall it should be easier to maintain.
@mbj mbj force-pushed the upgrade/regexp-parser branch 10 times, most recently from c8244ce to 6f90635 Compare January 6, 2023 22:01
* Not using circle CI for a long time
* Also most modern container setups have correct `/proc/cpuinfo` now.
@mbj mbj merged commit 51cf236 into main Jan 6, 2023
@mbj mbj deleted the upgrade/regexp-parser branch January 6, 2023 22:44
@jaynetics
Copy link
Contributor

  • The Parser::Expression class does not have a "nice" interface so a new mutator hierarchy is needed.

@mbj hi! what would a "nice" interface be from your point of view? (i maintain regexp_parser and am always interested in making it more useful.)

@mbj
Copy link
Owner Author

mbj commented Feb 20, 2023

@jaynetics Hola, yes I know you are the regexp parser author ;)

Whats comming now is all from memory:

My biggest issue with the regexp_parser gem as is, is that I have to specify "redundant knowledge" each time I construct a node.

If I choose to instantiate a node, and its type uniquely identifies the syntax already, I've yet to provide that redundant syntax when calling the constructor, and maintain that redundant information at the call side. In fact the public API for creating nodes allows me to create invalid regexp nodes.

Example:

Regexp::Expression::Group::Passive.construct(
  text: '(?:' # '(?:' is the "only" valid value for `text` for this node, its entirely redundant that a call side using #construct has to passs it.
)

I do recognize that for many nodes, the text is not an 1:1 relationship between the class and its runtime text values. But as I can also do: Passive.construct(text: 'fundamentally_invalid syntax .( issue') here and than affect: #to_r negatively should proof my point. I simply should NOT be allowed to set a text where there is no different value possible, ever.

An AST node constructor IMO, should only ever take arguments where the the value actually is allowed to be different at runtime.

The next issue is the mutability concerns, I have to deep clone every object as there is no native interface to create a mutated copy. This leads to noise in my code.

There where some smaller concerns I had, but do not recall anymore.

BTW I hope you see my verbose comments here not as negative, I'd love to receive equivalent critism on the public interfaces of my stuff ;). Worst case I disagree with the critics and have improved my mental model in the process. Best case: I discover I'm wrong and can improve.

Thanks for your work on regexp_parser, without it mutant would simply have not regexp support! Maintaining mbj/unparser as major "infra work" for mutant is "enough", I'd not be capable to "also" do a regexp tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants