-
Notifications
You must be signed in to change notification settings - Fork 17
feat(#556): auto-fix architecture #584
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
base: master
Are you sure you want to change the base?
Conversation
|
@yegor256 @h1alexbel I know the pipeline isn’t passing right now, but could you take a look at the idea behind it? |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
| ) | ||
| ) | ||
| ) | ||
| .orElseGet(Fix::new) |
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.
@Marat-Tim maybe we should throw an exception in this case? Fix::new only will make things more complicated
|
|
||
| import com.github.lombrozo.xnav.Xnav; | ||
|
|
||
| public class Create { |
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.
@Marat-Tim these classes should be package private. We don't want to expose them to the client
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
|
|
||
| public class File { |
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.
@Marat-Tim File is a name of the well-known Java class. Let's rename this class to avoid confusion
| } | ||
|
|
||
| String get() { | ||
| return xnav.attribute("source").text().orElse(""); |
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.
@Marat-Tim XMIR does not have @source attribute anymore, you should use org.eolang.parser.ObjectName instead
| * | ||
| * @return Fix info. | ||
| */ | ||
| Fix fix(); |
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.
@Marat-Tim to be honest, it is hard to review Fix architecture without unit tests. For now, I don't really understand how it works. So, I suggest to create unit tests, or even test YAML stories (the way we did in LtByXslTest#testsAllLintsByEo and WpaLintsTest#testsAllLintsByEo) to see how it works in practice (EO before fix vs. EO after fix).
I didn’t find any clear standard format for describing code fixes, so I went with the LSP
DocumentChangesformat. It’s reasonably clean, supports all kinds of edits, and integrates well with our LSP server. Foreoc, though, we’ll likely need to parse and apply changes manually(question). Hopefully, it won’t be too complexI originally planned to use the
lsp4jlibrary, hoping it would support easy (de)serialization. But it uses a complexGSONsetup, andgson-xmlfailed to handle it. So I decided to recreate theDocumentChangesstructure with our own data classesAdded the
fixpackage to hold data classes describing fixes3.1. Initially, I made all classes interfaces with
Defaultimplementations(similar to theDefectdata class).But later realized that was overkill - no one would realistically extend these. So I switched to plain Java data
classes. Now, creating a fix is simpler:
instead of
3.2. I’m aware that we aim to keep the public API as minimal as possible, and a public fix package doesn't quite align with that. Still, putting all these classes into
org.eolang.lintswould look messy. Likewise, movingFixtoorg.eolang.lintsand nesting the rest as inner classes would be ugly. I considered keeping only interfaces and implementing them via anonymous classes where needed, but that would be too verbose. This might be worth a separate discussion here.By default, linters still don’t offer fixes and behave as before. But to add fix support:
4.1. For Java-based linters, just pass fix data into the constructor(like this)
4.2. For XSL linters, move the message to a
<message>tag and put the fix info in an<edit>tag(like this). I assumed XSL linters won’t need file creation or multi-file edits, so one edit per case should be enough