Skip to content

InlineExpectationsTest: Allow URLs as values #8530

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RasmusWL
Copy link
Member

For example,

requests.get("http://example.com") # $  clientRequestUrlPart="http://example.com"

would not have been allowed previously, since the // part of the value for the URL would have caused the rest of the line to be treated as a normal (non-interpreted) comment.

Requiring a space before the // allows this specific example, while still support all the existing uses of // to break expectation tests, which is usually to add a comment explaining why the annotation is like it is.

For example,

```py
requests.get("http://example.com") # $  clientRequestUrlPart="http://example.com"
```

would not have been allowed previously, since the `//` part of the value
for the URL would have caused the rest of the line to be treated as a
normal (non-interpreted) comment.

Requiring a space before the `//` allows this specific example, while
still support all the existing uses of `//` to break expectation tests,
which is usually to add a comment explaining why the annotation is like
it is.
@RasmusWL RasmusWL force-pushed the inline-expectation-tests-allow-urls branch from f31ff96 to 6d3ad95 Compare March 24, 2022 10:01
@RasmusWL
Copy link
Member Author

@dbartol just wanted to check whether you would be OK with this change? (before pinging all the other language teams)

@owen-mc
Copy link
Contributor

owen-mc commented Mar 24, 2022

Note that codeql-go also uses inline expectations. Would it be possible to update it there as well? I spent a while bringing it up to date about 6 months ago, so it should be pretty easy.

@RasmusWL
Copy link
Member Author

Note that codeql-go also uses inline expectations. Would it be possible to update it there as well? I spent a while bringing it up to date about 6 months ago, so it should be pretty easy.

Sure thing, I'll do if the overall idea is accepted 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants