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

Emit warning for unmatched #region/#endregion #3308

Closed
debonte opened this issue Sep 8, 2022 · 5 comments
Closed

Emit warning for unmatched #region/#endregion #3308

debonte opened this issue Sep 8, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@debonte
Copy link
Contributor

debonte commented Sep 8, 2022

We support folding of #region/#endregion pairs, but when there's an unmatched #region or unmatched #endregion our behavior will be surprising to the user.

For example, in this scenario, the user apparently omitted an #endregion for the inner #region. The result is that the inner #region has a folding control >, but the folding range unexpectedly ends at the "outer" #endregion:

#region outer
    ...
    #region inner
    ...
#endregion

This can also be caused by a typo. For example, in the following case the region is not foldable because #endregion is misspelled:

#region foo
...
#endregio

TypeScript doesn't seem to warn for this issue, but C# flags these types of situations with "CS1038: #endregion directive expected" for an unmatched #region and "CS1028: Unexpected preprocessor directive" for an unmatched #endregion.

@rchiodo
Copy link
Contributor

rchiodo commented Nov 30, 2022

This issue has been fixed in prerelease version 2022.11.41, which we've just released. You can find the changelog here: CHANGELOG.md

@rchiodo rchiodo closed this as completed Nov 30, 2022
@bkkm78
Copy link

bkkm78 commented Jan 29, 2023

Fixing this by emitting a warning does not look like a good idea to me. The #region/#endregion directive is specific to vscode and not standard or conventional for Python in general. For someone not aware of this vscode feature, it is completely possible for them to write "#region" to refer to something that is not the following code block. For example, they may be talking about part of an image when the code snippet is about processing images. When viewing code like this in vscode, the warning from pylance about #region/#endregion is irrelevant and unhelpful.

At least let people turn off this type of warning in the settings.

@erictraut
Copy link
Contributor

Perhaps we should just remove this diagnostic altogether. Is it providing enough value to justify its existence? If someone forgets to provide an #endregion marker, they will simply not get region-based folding for that particular region. What was the justification for adding a diagnostic for this condition in the first place? It sounds like something that is more appropriate for a linter tool than a type checker.

@debonte
Copy link
Contributor Author

debonte commented Jan 30, 2023

If someone forgets to provide an #endregion marker, they will simply not get region-based folding for that particular region.

That's only true for the last #region in the file. Consider the following scenario where the ... lines potentially represent many lines of code.

#region 1
...
#User intended to put an endregion here but forgot, deleted it, moved it with cut/copy paste, etc.
...
#region 2
...
#endregion

This will result in two unexpected behaviors:

  1. VS Code will only display one region-related folding icon/button in the gutter, on the line for region 1. Region 2 won't have one because we didn't find a matching #endregion.
  2. When the user folds region 1, it will fold all of the code between #region 1 and the bottom #endregion.

It sounds like something that is more appropriate for a linter tool than a type checker.

Pylance isn't strictly a type checker though, right? It's also a language server. This diagnostic was intended to complement Pylance's support of #region/#endregion folding, so users can figure out what is wrong when they encounter unexpected behaviors like the ones described above.

I agree that this diagnostic provides very little value for Pyright users, which is why I was suggesting that the default behavior in Pyright and Pylance should be different.

@debonte
Copy link
Contributor Author

debonte commented Jan 30, 2023

it is completely possible for them to write "#region" to refer to something that is not the following code block. For example, they may be talking about part of an image when the code snippet is about processing images.

@bkkm78, we're actually discussing that specific issue over at #3857

The #region/#endregion directive is specific to vscode and not standard or conventional for Python in general.

It's also supported by PyCharm for what that's worth. I'm not sure how many other Python editors / language servers support it.

I agree that #region is in an awkward state where it's not part of the language standard but has broad usage as a convention.

I asked Brett Cannon about writing a PEP to standardize the behavior of #region/#endregion and he said:

Since it is entirely a tool-specific thing I don't think a PEP is necessary, but it could be discussed at https://discuss.python.org/c/editor-integration/27 to see if we can get some consensus among editors.

PyCharm also has this issue where comments containing the word "region" are treated as the start of a folding region. If we had a proposal for addressing that, beyond disabling the feature, I would be happy to start a discussion with PyCharm and others on discuss.python.org so the behavior is consistent across editors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

5 participants