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

Initial work on rep * #24

Open
wants to merge 7 commits into
base: 12-regex-ops
Choose a base branch
from

Conversation

davidnormo
Copy link

Just opening this PR now to get some feedback. A quick question: what name did you want to use for * - I'm leaning towards zeroOrMore but that is quite verbose. There is also rep used in places and kleene.

Work in progress.

star [:k1 :k2] [:k1 :k2] nil
star [:k1 :k2 "x"] ::s/invalid '[{:pred keyword?, :val "x" :via []}]
star ["a"] ::s/invalid '[{:pred keyword?, :val "a" :via []}]
*/
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just used the clojure tests as a starting point.

@coveralls
Copy link

coveralls commented Apr 12, 2017

Coverage Status

Changes Unknown when pulling a85864e on davidnormo:12-regex-ops into ** on prayerslayer:12-regex-ops**.

@davidnormo
Copy link
Author

#12

@prayerslayer
Copy link
Owner

prayerslayer commented Apr 12, 2017

Wow, thanks a lot for taking the time! It'll take a while until I can look over it thoroughly, possibly next Monday?

I named * kleene because that's how they call this operator in the regex derivative paper. For sake of consistency I'd like to stick with that in the js.spec code, but we don't necessarily have to export it under this name.

As for what's missing, but you probably know this yourself, I'd like to see support for other specs (like map) and especially other combining specs like and/or as well as nilable.

Do you think we can reuse your code to build +? Maybe that's already a step too far ahead, but good to keep in mind, I don't know.

@davidnormo
Copy link
Author

Sure, I'm going try and tidy up a bit, so hopefully it's in a reasonable state for you to review.

Yep, I need to add the other bits and pieces in (and/or etc).

I'll have a think about +

@davidnormo
Copy link
Author

I think I'm a bit closer, at least kleeneDeriv seems to match the logic in the paper. But still trying to work out how to wrap the return value (see running test).

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Changes Unknown when pulling 503ffda on davidnormo:12-regex-ops into ** on prayerslayer:12-regex-ops**.

@davidnormo
Copy link
Author

@prayerslayer So I've finally got kleene in a working state - will need a bit of clean up.

I've found a couple of problems though:

  1. I'm confused around your usage of null and undefined. Clojure only has nil and doesn't have undefined args such as: (s/conform (s/* int?)) => ArityExecption - missing 2nd arg. However in JS such a call will result in the 2nd arg becoming the value undefined. Are we treating null and undefined the same? IMO we should treat null as the value and undefined as an error. For example:
conform(kleene(int)) // invalid
conform(kleene(int), undefined) // invalid
conform(kleene(int), null) // []
conform(kleene(int), []) // []
conform(kleene(int), [null]) // invalid
conform(kleene(nilable(int)), [null]) // [null]
conform(kleene(int), [undefined]) // invalid
conform(kleene(nilable(int)), [undefined]) // invalid
  1. In some places js.spec diverges from clojure.spec - I'm not sure if that is intentional or not? For example named maybe regex? And regexes not failing when given extra input?

@prayerslayer
Copy link
Owner

Are we treating null and undefined the same?

That's what I tried to do until now, yes. Both have the semantic "value does not exist." But I'm open for arguments to change it, it might not hold well with nilable, repeatable things.

In some places js.spec diverges from clojure.spec

Could you provide a specific example? I'm not sure I follow.

@arichiardi
Copy link
Collaborator

Hello folks! any progress on this one?

@davidnormo
Copy link
Author

@arichiardi I've not had time to look at this and doubt I will in the near future. If that changes I'd love to help out more. Apologies!

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.

None yet

4 participants