Refactoring: remove library interfaces #157
Conversation
lefessan
force-pushed
the
z-2024-02-01-remove-library-interfaces
branch
from
d5ebf47 to
eb1016f
Compare
Library interfaces are generally a wrong good idea: it hides modules that could be useful later, it renames modules whose source files are then harder to locate, it may contain code that should be in other modules. So, we must avoid them.
lefessan
force-pushed
the
z-2024-02-01-remove-library-interfaces
branch
from
f87c264 to
03cf06f
Compare
There was a problem hiding this comment.
As a whole this PR gathers some changes I adhere to, and some I am strongly opposed to:
- I am against removing interface module in libraries that mostly define types and associated utilities (like the parse tree): I do not want to have to type
Typesall the time, while I still want to mention where the types are from in other libraries; - I am also against the renaming of many module names for which the prefix indicates where the module belongs to (
Src_andPreproc_in the preprocessor,data_in the types for representing the COBO data, etc). When editing files in various libraries at the same time, those prefixes are very useful. Complicated changes in the code often imply editing several libraries at the same time, so I think on the long run that outweighs the additional on-boarding cost. - I am in favor of removing the
includes from interface modules (so, havingMainmodules that gather principal points).
All in all, I'd rather opt for a shortened version of this PR where the only remaining interface modules would be a list of module bindings that only remove their respective library-specific prefix (like Lsp_).
There was a problem hiding this comment.
all_types.ml would fit better with our naming scheme.
| @@ -11,7 +11,7 @@ | |||
| (* *) | |||
| (**************************************************************************) | |||
|
|
|||
| open Lsp_project.TYPES | |||
| open Project.TYPES | |||
There was a problem hiding this comment.
Note Lsp_project defines an LSP-specific view on more general SuperBOL projects. Only having Project makes this quite unclear.
| @@ -32,9 +32,9 @@ module TYPES = struct | |||
| } | |||
| and checked_doc = Cobol_typeck.Outputs.t | |||
| and rewinder = | |||
| (Cobol_ptree.compilation_group option, | |||
| (Cobol_ptree.Types.compilation_group option, | |||
There was a problem hiding this comment.
cobol_ptree is a library that should define types and associated visitors and printers. For that one having tot type Types everywhere is very cumbersome. I am highly in favor of keeping the neat interface for that one.
| Cobol_typeck.compilation_group ~config ptree |> | ||
| Cobol_typeck.translate_diagnostics ~config |> | ||
| Cobol_typeck.Engine.compilation_group ~config ptree |> | ||
| Cobol_typeck.Engine.translate_diagnostics ~config |> |
There was a problem hiding this comment.
Maybe Main instead of Engine for cobol_typeck as well.
| @@ -17,14 +17,15 @@ open Cobol_common (* Visitor *) | |||
| open Cobol_common.Srcloc.INFIX | |||
|
|
|||
| open Lsp_completion_keywords | |||
| open Lsp.Types | |||
|
|
|||
| module POSITION = Lsp.Types.Position | |||
There was a problem hiding this comment.
Note that POSITION is highly confusing and makes the code below less readable. In this case I'd just remove that definition and always qualify with Lsp.Types..
There was a problem hiding this comment.
I'd keep Server_loop for that one, that's more precise.
|
FWIW, the subject is not whether we keep some package interfaces or not, we will get rid of all of them for readability and navigation issues. The question is more how to make it while keeping some consistency and ease of programming and qualifying things. |
Library interfaces are generally a wrong good idea: it hides modules that could be useful later, it renames modules whose source files are then harder to locate, it may contain code that should be in other modules.
So, we must avoid them.