-
Notifications
You must be signed in to change notification settings - Fork 194
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
Module file name rule autofix #2162
Module file name rule autofix #2162
Conversation
Will it work on closing module name like:
|
No, but I'll look into it.
…On Tue, 16 Apr 2024, 10:38 pm Jonathan Drolet, ***@***.***> wrote:
Will it work on closing module name like:
module a();
endmodule : a
—
Reply to this email directly, view it on GitHub
<#2162 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVJSIZ44D7X3YH6A2IJ7VTY5ULTDAVCNFSM6AAAAABGJIEAJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYHE4DSOJXHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I checked this out of curiosity, sharing in case it is useful @sconwayaus
Edit: Just in case you want to quickly check autofix stuff, after recompiling
|
…onwayaus/verible into module_file_name_rule_autofix
…onwayaus/verible into module_file_name_rule_autofix
…onwayaus/verible into module_file_name_rule_autofix
Now updates the endmodule identifier (if present). I should point out is the rule now raises a violation for all modules (excluding inner modules) if no matches are found. The rational is that the checker doesn't know what module to rename with the autofix, so it now suggests an autofix for all modules. |
I don't understand this change. I see how defaulting to the last module isn't particularly "smart" but I don't see any strict improvement over the default.
It does know, right? it will apply/suggest the autofix for the last module in the file. By the way, this change can lead to very strange situations if you apply all suggested fixes by the tool:
|
The intention was to present the user with all the options, and let the user choose the "right" moudle to rename. Seems to work well in VSCode. I do see your point on users accepting all autofixes from the CLI. If you feel strongly that this was a poor choice I can revert to the original behaviour. |
I am wondering if the filename should take the precedence here, imposing the name to the module. Usually one would write stuff, comes up with a better name of a module and then rather would have the filename be renamed to match the new name of the module. I am not sure though if that can be auto-fixed easily (I think there is no easy 'rename file edit' that can be send to the LSP), and it would probably also create other issues like having to update build-systems, having to deal with filenames that already exists etc. Changing the module name to the filename is simple to implement, but does it also adhere to the principle of least surprise ? |
I disagree about ever changing the filename on disk. This could create conflict if two separate files have a module with the same name (like, if you copy a.sv to b.sv). It is always safe to rename the module to match the filename. |
Agree, this is how I tend to work, but sometimes I go the other way too. Using the Language Server a bit more I really should rename the symbol so usages get fixed too. With this in mind this change is of little value for my own work flow, and I'm not sure it's of much use to others. Happy to make changes to this PR if others think it would be useful, otherwise I might close it next week without merging. |
It is somewhat supported: #2081 but the "machinery" doesn't really run so smoothly yet. I still think the autofix is valuable (more so if there is a label matching the module). I wouldn't get too caught up on discussing whether this is that useful. There are autofixes to remove trailing whitespaces, redundant semicolons... I think the small change is worthwhile, although I wouldn't change the old behaviour of only complaining about the last module. |
Reverted back to raising violation on the last module. |
If we offer potential fixes, maybe we could also offer just adding a waive-comment (watching myself: that is typically what I actually do when I run into this lint message)
Having multiple choices for autofixes vs. a single one can also help the user see that this is something to choose from, not an ultimate 'this is the only possible fix' way. I will probably have time to review this more thoroughly on Friday, but good work so far! I love seeing these improved autofixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo, looks like it is finally Friday and I have time to review :)
Added a simple autofix for [module-filename] lint rule to rename the module to match the filename.