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

Add support for new absence operator (#33) #34

Merged
merged 1 commit into from
Jul 10, 2017
Merged

Add support for new absence operator (#33) #34

merged 1 commit into from
Jul 10, 2017

Conversation

jaynetics
Copy link
Collaborator

No description provided.

@ammar
Copy link
Owner

ammar commented Apr 3, 2017

Hi @Janosch-x . Thanks for this and sorry about the delayed response.

My initial thinking was this new "operator" is not really a "group" and thus had started investigating an alternative approach. However the fact that the new construct allows for any sub-expression to appear as a exp makes using the group approach easier because it will use the existing scanner recursion. for example in a case like (?~abc|def|[ghi])

So, this is probably the best approach now and it looks good.

Cheers and thanks again!

@ammar
Copy link
Owner

ammar commented Apr 3, 2017

@Janosch-x

On further review I noticed a couple of things:

  • Need to double check the addition of Absent to Group::All based on where it is used (see ruby 1.8.6 syntax file)
  • The new type/token (:group/:absent) should be added to the ruby 2.4.1 syntax file with a call to implements.

@jaynetics
Copy link
Collaborator Author

Hi @ammar,

thanks for the feedback.

I also pondered whether it really is a group, ending up with the same conclusion as you did. To be sure I even asked the author, and he does see it is a group :)

Need to double check the addition of Absent to Group::All based on where it is used (see ruby 1.8.6 syntax file)

This is indeed a mistake. Do you have a preferred solution? I think All should contain Absent, because it doesn't seem right to handle version differences in Token::Group. Maybe we could write Group::All - [Group::Absent] in the 1.8.6 syntax file? Of course that is not very pretty either ...

@jaynetics
Copy link
Collaborator Author

Or maybe remove Group::All and list all the "classical" group types individually in the 1.8.6 file?

@jaynetics jaynetics changed the title Add support for new absent operator (#33) Add support for new absence operator (#33) Apr 17, 2017
@jaynetics
Copy link
Collaborator Author

@ammar In accordance with other token files I've removed the new type constant from the All array and called implement on it separately in the 2.4.1 file.

There is also a syntax file test and a parser test for it now.

Lastly, to match the naming in Onigmo, I have changed the name from "absent" to "absence".

@ammar
Copy link
Owner

ammar commented Apr 27, 2017

@Janosch-x Sorry for not responding sooner. I recently had an accident and have an arm and a shoulder in cast. Hope to be able to work again soon

@jaynetics
Copy link
Collaborator Author

@ammar I'm sorry to hear that. Please don't feel pressurised by my landslide of PRs. There is no urgency there at all, I had just been bored. Wishing you a good recovery!

@ammar
Copy link
Owner

ammar commented Jul 10, 2017

Hi @Janosch-x, thanks for looking into the classification of the operator and the updates. Again, I'm very sorry for the very late response.

I think this looks good and I would like to merge it.

Cheers!

@ammar ammar merged commit 1a4e5ee into ammar:master Jul 10, 2017
@ammar
Copy link
Owner

ammar commented Jul 10, 2017

Included in release 0.4.4

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