search

databrickslabs / ucx

Automated migrations to Unity Catalog
Other
229 stars 79 forks source link

[FEATURE]: Improve linter/fixer model #1211

Open ericvergnaud opened 7 months ago

ericvergnaud commented 7 months ago

Is there an existing issue for this?

Problem statement

Code migration agents such as SparkSql rely on multiple inheritence from Linter and Fixer, which are separate classes. This separation is artificial. In practice, you can't have a Fixer without a Detector (currently named Linter). As a result, the Languages class maintains 2 separate lists for linters and fixers, and holds the responsibility of ensuring there exists a Fixer for each Linter, which it should not hold.

Proposed Solution

Create an AdviceHandler class that inherits from Linter and Fixer and have code migration agents inherit from it.

Additional Context

No response

nfx commented 7 months ago

Why not just making class Fixer(Linter):?

ericvergnaud commented 7 months ago

Why not just making class Fixer(Linter):?

I guess because Fixer is not a specialization of Linter ? There's no hard rule for this, but in this particular case, I think that composition is more appropriate than inheritance. I might be biased by my single-inheritance + multiple interfaces background. In Java, AdviceHandler interface would extend Linter and Fixer, and Fixer interface would certainly not inherit from Linter interface.

nfx commented 7 months ago

@ericvergnaud we still need to see if we can change a portion of the code without reconstructing the entire syntax tree (think of comments, formatting, etc...), which means that Fixer interface is not final and may change. That may require Fixer to couple on a certain message codes.

ericvergnaud commented 7 months ago

That may require Fixer to couple on a certain message codes

Not sure I follow, can you provide an example ?