Skip to content

Conversation

@ono-max
Copy link
Contributor

@ono-max ono-max commented Dec 5, 2025

Before

"data": {
  "markers": [
    { "name": "name", "value": "foo" },
    { "name": "args", "value": "()" },
    { "name": "kwargs", "value": "{}" }
  ],
  "lineNumber": 3
}
"data": {
  "markers": [
    { "name": "name", "value": "parametrize" },
    { "name": "args", "value": "('y', [2, 3])" },
    { "name": "kwargs", "value": "{}" },
    { "name": "name", "value": "parametrize" },
    { "name": "args", "value": "('x', [0, 1])" },
    { "name": "kwargs", "value": "{}" }
  ],
  "lineNumber": 12
}

After

"data": {
  "markers": [{ "name": "foo", "value": "" }],
  "lineNumber": 3
}
"data": {
  "markers": [
    { "name": "parametrize", "value": "('x', [0, 1])" },
    { "name": "parametrize", "value": "('y', [2, 3])" }
  ],
  "lineNumber": 12
}

@ono-max ono-max requested a review from kohsuke December 5, 2025 07:31
@ono-max
Copy link
Contributor Author

ono-max commented Dec 5, 2025

c.c. @NicuPascu

@ono-max ono-max force-pushed the change-metadata-format branch from 93ba07c to 878c217 Compare December 5, 2025 07:34
Copy link
Contributor

@kohsuke kohsuke left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this so quickly!

  • In Python's function calling convention, arguments can be partially positional (what Python calls args) and partially keyword based (what Python calls kwargs), for example f(1,2,a=3) so assuming this metadata follows the same scheme, this code could see both of them at the same time. If that happens, this simply drops the positional arguments, which would be hard to notice. If you choose to handle this, I suppose the easiest is to just concatenate both args and kwargs. Python invocation syntax (1,2,a=3) would be probably nicer in terms of visualization, but it's bit more effort processing. Maybe Claude Code could write that for us?
  • I believe the test dependency mechanism that some customers rely on are built on this metadata. If so, I'd imagine we need to update the server side first to be able to deal with both forms, before we roll out this change.

Copy link

@NicuPascu NicuPascu left a comment

Choose a reason for hiding this comment

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

LGTM 🐝

Can we go through the DB and update the format of the all the old markers as well or should we support both data structures?

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.

4 participants