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

Use the newly introduced is not operator #9777

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

0stam
Copy link
Contributor

@0stam 0stam commented Aug 17, 2024

Use the newly introduced is not operator instead of not (...) is.

Running the following regular expression: (^|\`\`)\s*(if|while|.*=)(.)* not (|.* )is I have found only one occurrence of the outdated code in the entire documentation.

@0stam 0stam marked this pull request as draft August 17, 2024 21:22
@0stam 0stam force-pushed the is-not-conversion branch from 9e4cb5c to c47fb55 Compare August 17, 2024 21:28
@0stam 0stam marked this pull request as ready for review August 17, 2024 21:29
@0stam 0stam changed the title Use newly introduced is not operator Use the newly introduced is not operator Aug 17, 2024
@0stam 0stam force-pushed the is-not-conversion branch from c47fb55 to 955417b Compare August 17, 2024 22:17
@skyace65 skyace65 added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Aug 21, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Aug 22, 2024

I wonder if there's any not (...) is in the class reference, too...

In any case, it's an innocent addition. Although, the simplification is... simpler, yeah, but may serve a different purpose. Assignments to variables and early returns like the code snippet above are pretty nifty.

@0stam
Copy link
Contributor Author

0stam commented Aug 22, 2024

I wonder if there's any not (...) is in the class reference, too...

After all the searching, I have read the documentation guidelines. It turns out that, to make everything more concise, you should avoid using typed code. So it makes sense that the operator is not used outside of the typing tutorial, and I don't expect to find it in the class reference.

At first I wanted to replace the operator in the expression, but then realised that this page also introduces the is operator itself, and introducing both at the same time could be misleading to someone.

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Another PR, #10085, also tries to document is not in this section (though that PR only replaces the existing usage). One of these two PRs should be accepted.

Comment on lines 282 to 285
You can also simplify the expression using the ``is not`` operator::

if body is not PlayerController:
push_error("Bug: body is not PlayerController")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can also simplify the expression using the ``is not`` operator::
if body is not PlayerController:
push_error("Bug: body is not PlayerController")
You can also simplify the expression using the ``is not`` operator::
if body is not PlayerController:
push_error("Bug: body is not PlayerController")

This needs to be indented one level deeper, so it appears in the note block, since it is conceptually related to the earlier example.

You should also relocate this new is not example to after the initial is example and before the assert() example. Note that you'll have to adjust the existing or ``assert()`` statement:: line to make sense.

@0stam 0stam force-pushed the is-not-conversion branch from 955417b to 8d4ab7a Compare November 6, 2024 21:55
@0stam 0stam force-pushed the is-not-conversion branch from 8d4ab7a to 6ef6ad8 Compare November 6, 2024 22:08
@0stam
Copy link
Contributor Author

0stam commented Nov 6, 2024

Thanks for the review, I have applied all the feedback. I have also replaced an incorrectly used "expression".

We don't have a separate documentation for the is keyword, so I would personally stick with explaining the basic version before introducing is not, but I'm no expert on the topic.

The other PR also suggest removing parentheses from if not (body is PlayerController):. It's technically correct, because is has a higher precedence than not, but it's not a very common knowledge (at least I had to check it) and I believe it's much easier to understand in the current version, especially if you see it for the first time.

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Looks good now. This PR is a better way to document is not than the alternate PR #10085.

Keeping the existing parentheses in the if not (body is PlayerController): line makes sense to me, since it increases clarity if you don't know all the precedence rules. But I'm not an expert on the GDScript style standards we use for docs examples.

@mhilbrunner mhilbrunner merged commit ff46ea4 into godotengine:master Nov 17, 2024
1 check passed
@mhilbrunner
Copy link
Member

Merged. Thanks and congrats on your first merged contribution! Sorry this one took a bit :)

@0stam
Copy link
Contributor Author

0stam commented Nov 20, 2024

Thank you too. I'm glad I could help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants