Skip to content

Allow plain Python literals in regexp function wrappers#1493

Open
renato2099 wants to merge 4 commits intoapache:mainfrom
renato2099:renato2099/1446
Open

Allow plain Python literals in regexp function wrappers#1493
renato2099 wants to merge 4 commits intoapache:mainfrom
renato2099:renato2099/1446

Conversation

@renato2099
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #1446.

Rationale for this change

Make regex functions more pythonic

What changes are included in this PR?

Changes to regex functions such that we can improve their usability by making them more pythonic

Are there any user-facing changes?

Regex functions are more pythonic now

Copy link
Copy Markdown
Member

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

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

This is a nice addition. I think we should update the docstrings to not have lit() since we can take these values directly now.

string: Expr, pattern: Expr, start: Expr | None = None, flags: Expr | None = None
string: Expr,
pattern: Expr | str,
start: Expr | str | None = None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think start should be Expr | int | None not str

Comment on lines +1598 to +1599
start: Expr | str | None = None,
n: Expr | str | None = None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we want these as int not str

start: Expr | str | None = None,
n: Expr | str | None = None,
flags: Expr | str | None = None,
sub_expr: Expr | str | None = None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the capture group (sub_expr) is also an int. We should verify

@timsaucer
Copy link
Copy Markdown
Member

Also worth noting is I've been working on a skill that will do these in a more automated way: #1484 It might be worth comparing what you came up with vs the agent.

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.

Improve usability of regexp and other functions

3 participants