Skip to content

InlineExpectationsTest: Allow URLs as values#8530

Draft
RasmusWL wants to merge 1 commit intogithub:mainfrom
RasmusWL:inline-expectation-tests-allow-urls
Draft

InlineExpectationsTest: Allow URLs as values#8530
RasmusWL wants to merge 1 commit intogithub:mainfrom
RasmusWL:inline-expectation-tests-allow-urls

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