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

feat(cdp): allow substring without length param #28746

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MarconLP
Copy link
Member

@MarconLP MarconLP commented Feb 14, 2025

Problem

If you don't provide a length parameter, we should return the entire string starting from the provided start index.
https://posthoghelp.zendesk.com/agent/tickets/24971

Changes

  • don't require the length param, and fallback to returning the entire string starting from the provided start index
  • Check why the nodejs test is failing

Does this work well for both Cloud and self-hosted?

How did you test this code?

added test

@MarconLP MarconLP marked this pull request as draft February 14, 2025 18:30
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR modifies the substring function across Python and JavaScript implementations to make the length parameter optional, defaulting to returning the entire remaining string from the start index.

  • Modified substring function in /common/hogvm/python/stl/__init__.py to handle optional length parameter
  • Added test case in /common/hogvm/__tests__/stl.hog verifying substring behavior without length parameter
  • Updated JavaScript STL implementation in /posthog/hogql/compiler/javascript_stl.py for consistency
  • Needs investigation into failing Node.js test mentioned in PR description
  • Added .cursorignore file to exclude .env from editor (seems unrelated to main changes)

4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -1214,7 +1214,7 @@
[],
],
"substring": [
"""function substring(s, start, length) {
"""function substring(s, start, length = s.length - start + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: default length calculation s.length - start + 1 will return one character too many due to 1-based indexing

Suggested change
"""function substring(s, start, length = s.length - start + 1) {
"""function substring(s, start, length = s.length - start) {

@MarconLP MarconLP requested a review from mariusandra February 17, 2025 08:37
@MarconLP MarconLP marked this pull request as ready for review February 17, 2025 08:41
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

I wouldn't put complex expressions in function arguments

common/hogvm/__tests__/__snapshots__/stl.js Show resolved Hide resolved
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