Dismiss of CST.Types module

[wclr]

This PR is not a fix for a bug/issue or an asserted proposal for a change.

I will explain my line of thoughts here that makes me propose/discuss this.

Generally, I consider *.Types and similarly genericly named shared modules (e.g. *.Monad) to be an unintentional, though widely established, poor practice that leads to more complex module structure design and relationships.

Such "common" modules issue sometimes is refered to horsontal vs vertical organization, though it may not be very intuitive why having such modules is not the best design choice and may seem more of subjective opinion than justified objection (though in the link above some valuable and important reasons are mentioned).

More objectively, when such modules are in place this often means violation of the following module hierarchy principle, which implies that if there are modules A and A.X then A.X may depend on A, but A can never depend on A.X. According to this principle, semantically A.X extends or specifies A in terms of X and not that A.X holds X part of A functionality. For example, consider Codec and Codec.JSON.

I would argue that violating this principle eventually leads to more complex and less intuitive designs, but following this simple design limitation allows for more problem specific and meaningful structure.

As a side note: as purescript doesn't support internal (not exposed) modules, I see an exception case for this only when dealing with such internal modules that hold indivisible core functionality intended to be hidden by structured external API (e.g. latest implementation of JSON modules).

I found this package in its current state to be intresting and obvious case of this problem, which is easy to fix. Based on the above considerations I made the following changes to the module structure of Purescript.CST:

• CST.Types - > CST (parent namespace to contain CST representations, which are "types" obviously)

• CST -> CST.Recovered (extends CST with parsing functionality, exposes PartialModule and RecoveredParserResult, not sure if the name Recovered here conveys intention the best/correct way, but probably it does)

• CST.Parser.Monad -> CST.Parser (implements and exposes Parser)

• CST.Parser -> CST.Parser.Recovered (extends CST.Parser in terms of parsing to Recovered result)

• CST.Range -> CST.Range and CST.Tokens (split into two independent modules)

• CST.Range.TokenList -> CST.TokenList

• Move print related functions to CST.Print:

  • printModule - from CST
  • printParseError - from CST.Errors

I believe that this change removes some ownership and responsibilities issues, makes the structure more problem-specific and some modules simpler. Although it may seem kind of "inconvenient" that the most required functionality like parseModule is not in the CST but nested in CST.Recovered space, but this reflects intention and problem stucture more corectly. It also shows that it is now easier to split the package into something that contains only CST abstractions and other things that are focused on parsing.

This is my view on the problem, I'm interested in what you think about the issue in general and specifically applied to this package.

[natefaubion]

It also shows that it is now easier to split the package into something that contains only CST abstractions and other things that are focused on parsing.

This isn't really a goal to me. The primary purpose of any sort of CST is to provide a way to parse source into that CST. Constructing the "safe" types outside of a parser is dubious. On an abstract level, sure you can make some distinction here. But the 95% use case of producing a CST should be parsing, so that is the primary entry point and focus.

[wclr]

I understand your goal and intention. My main concern was about the general structure, the possibility of splitting the package is not the major point here, it is just a consequence of the correct structure organization (though I understand that people may think that there is not such thing as "correct" structure thing). As for naming, instead of CST.Recovered it would probably be better to use CST.Parsing namespace for end-user parsing functions.

The first thing is about using generic names like Types, often people eventually put there not only actual types, but also some functions to work on them, so it becomes just a place for all the "common" stuff that we don't know where it belongs.

And second, about namespace dependency direction: A.X -> A vs A -> A.X when parent namespace depends on nested modules, that are supposed to be exposed too. I would argue that this contributes to more confusion in the module structure.

I wonder, do you see any problems with those (not specifically for this package)?