Skip to content

Improve support for Python arguments#3540

Merged
koesie10 merged 1 commit intomainfrom
koesie10/python-model-editor-args
Apr 8, 2024
Merged

Improve support for Python arguments#3540
koesie10 merged 1 commit intomainfrom
koesie10/python-model-editor-args

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Apr 5, 2024

This improves support for Python arguments by doing two things:

  • When there is a function definition like:
def foo(x, y)
  return x + y

It is possible to call the function as normal foo(1, 2), which works with Argument[0] and Argument[1]. However, it's also possible to call this like foo(1, y = 2) or foo(x = 1, y = 2), which doesn't work for those same values (since the function definition isn't always available). Therefore, this changes the generation of these arguments to Argument[0,x:] and Argument[1,y:].

  • When there is a function definition like:
def foo(x, /, y)
  return x + y

It is not possible to call this like foo(x = 5, y = 2), only as foo(5, 2) or foo(5, y = 2). So, unlike above, we should ensure that these arguments are not modeled as both Argument[0] and Argument[x:], but only as Argument[0].

The CodeQL query will return x/,y as the parameter string for this function. When there are multiple positional-only arguments, each argument name will be suffixed by /.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 marked this pull request as ready for review April 5, 2024 14:00
@koesie10 koesie10 requested a review from a team as a code owner April 5, 2024 14:00
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

The changes seem reasonable based on what you've described in the PR description.

Do you think it's worth adding some unit tests around this functionality?

@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Apr 8, 2024

Do you think it's worth adding some unit tests around this functionality?

Yes, good call. I'll add some unit tests in a follow-up PR.

@koesie10 koesie10 merged commit 9e729ba into main Apr 8, 2024
@koesie10 koesie10 deleted the koesie10/python-model-editor-args branch April 8, 2024 11:59
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