-
Notifications
You must be signed in to change notification settings - Fork 33
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
vint poorly handle functions and their scopes #128
Comments
This is true. But vint should ignore function definitions when trying to fix function scopes unless it is inside |
One way to achieve this is via the following patch:
If function declaration has scope prefix, then leave it untouched, otherwise it is okay for a function declaration to not have a scope at all, so return |
Hmm, Vim actually allows the |
Now I see that vint tries to do best with function scopes. You've done a great job! Many thanks. Though I have one more question: Please share your thoughts on this one if you have time. See also PR #129 |
Thanks for reporting! I think that an option to ignore autoload functions for the policy
What do you think this option? |
I think such option would be even better. This way backwards compatibility is also preserved so users who prefer their autoload functions to be explicitly scoped will get the warnings. |
I am not sure how to implement this. The straightforward way is to add a boolean parameter to |
In my design, a Policy class has a responsibility of suppressing their warnings. But now, ProhibitImplicitScopeVariable cannot know whether an identifier is autoloaded. So, we should export is_autoload_identifier to ProhibitImplicitScopeVariable. Step1: Export IdentifierClassifier.is_autoload_identifier(node) as ScopePlugin#is_autoload_identifier(node). Step2: In ProhibitImplicitScopeVariable#is_valid(identifier, lint_context), we can check whether the identifier is an autoload identifier via lint_context['plugins']['scope']. Step3: Export Linter#_config to Policy via lint_context. Step4: We can check the desired option "suppress_autoload" in ProhibitImplicitScopeVariable#is_valid. |
Thanks for the detailed info. I've updated my PR. Tested and works as intended on my machine. Please review. Update: it also passes tests now. |
Also I'd personally prefer to store a policy option as an instance attribute and update it if necessary from PolicySet#update_by_config. Again I am not sure how it would fit into your design. |
As you say, we should define the comment config syntax for each policies. But I feel this feature is not so important. I want to pend the issue until the such request is arrived. |
I am sorry, but I didn't understand what you've meant here. Could you rephrase/expand please? |
Oh... sorry it is my operation mistake. |
I meant that we should define the "comment syntax for police options". For example: " vint: ProhibitImplicitScopeVariable.suppress_autoload=true
" vint: ProhibitImplicitScopeVariable={supresd_autoload: true} But these examples seems not good to me. |
Hmm, I still don't quite understand what you've meant here since vint uses YAML configuration and your example is something different. Are we talking about different things? If we are speaking about YAML-based configuration, then the standard YAML notation will suffice I think. For example:
If you check my update_by_config branch it does things this way. |
@Kuniwak
but was only able to get this to work via the vintrc yaml file |
@vito-c Sorry, this feature is not implemented yet. |
Hello.
Consider the following VimL code:
vint complains:
The comment about
Scope of variable
is out of place here. Anti-pattern of Vim also says nothing about function scopes.Now consider the following code that describes a function that should be available globally:
Again vint complains:
Logically this function should have
g:
scope, but Vim actually forbids it:See
:h E884
for more info.While the previous behavior is still somewhat tolerable, the following code:
is valid for vint, while for Vim it is clearly not. This is really bad.
Please fix.
The text was updated successfully, but these errors were encountered: