google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.27k stars 204 forks source link

feat: introduce regexp package #369

Closed b5 closed 2 years ago

b5 commented 3 years ago

@essobedo is going to be heading up turning this code into a package that closes #241. We're using the re package from starlib as a starting point, but it needs a lot of work before it's ready for initial review.

@essobedo I think at a minimum we should:

essobedo commented 3 years ago

@b5 perfect, thank you

google-cla[bot] commented 3 years ago

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹī¸ Googlers: Go here for more info.

essobedo commented 3 years ago

@googlebot I consent

essobedo commented 3 years ago

Sorry for the delay, I was very busy these days.

FYI I added a split function since in practice it is frequently used.

@b5 I believe that it is ready for a first review since I addressed everything that needed to be done so, could you please "un-draft it" 🙏

@adonovan Could you please review it? Thx in advance 🙏

essobedo commented 3 years ago

@adonovan

This isn't correct: if the backslash is preceded by a backslash, then it should be legal. You will need to use a stripped down version of the scanning algorithm from the regexp package.

I double checked and unless I really missed your point, if I don't precede \C with a backslash, I end up with the error invalid escape sequence \C. I have just added two examples one with the patterns \d, \w and \s all preceded by a backslash to show that it works as expected so the code seems to be actually correct (as you can see here).

Regarding the code, I only changed the code to use a raw string literal instead of a string literal to avoid having to escape the backslash but the logic is the same.

essobedo commented 3 years ago

@brandjon could you please do the review 🙏 ? @adonovan seems to be busy and/or not available

adonovan commented 3 years ago

Sorry for the delay, will take a look presently.

essobedo commented 3 years ago

@adonovan remarks addressed please check again

essobedo commented 3 years ago

@adonovan remarks addressed and questions answered, please check again

essobedo commented 3 years ago

@adonovan May I have a feedback please? 🙏

adonovan commented 3 years ago

Let's get @brandjon to opine on the API before we go into details of the implementation.

essobedo commented 3 years ago

Hi @brandjon, any feedbacks to give please 🙏 ?

essobedo commented 2 years ago

Let's try to finish it 😄 @brandjon Hi, thank you for your remarks, I've just pushed a new version, could you please check again 🙏 ?

essobedo commented 2 years ago

@brandjon Could you please check again? 🙏

essobedo commented 2 years ago

@adonovan @brandjon any remarks about this PR?

johntdyer commented 2 years ago

Any update here folks?

luispadron commented 2 years ago

Checking back a year later 😅 any update here? Is this PR blocked by something?

essobedo commented 2 years ago

@luispadron yes this PR is blocked because unfortunately there is no more reviewers in this project 😞

essobedo commented 2 years ago

@luispadron I guess we need to consider that this project is in maintenance mode and no more features will be accepted

essobedo commented 2 years ago

@b5 I guess you can close it

b5 commented 2 years ago

Dang. Frustrating. Ok closing now

jawilking commented 10 months ago

@luispadron I guess we need to consider that this project is in maintenance mode and no more features will be accepted

So is this project dead now?

adonovan commented 10 months ago

No, Starlark-go is very much alive, but time is precious and a regexp package is nowhere near our highest priorities. I'd still be willing to approve a PR to add the right API, but I don't know what that looks like at the moment and it would be a shame to add something that is too closely coupled to the underlying Go implementation to be implemented in Java or -Rust (as we may have done with the time package).

Until such time as a standard regexp package is added, you can of course fork the code in this PR in your own project.

snehal520 commented 4 months ago

Is there any plan of adding regexp to starlark anytime soon ?