Skip to content
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

Add module hierarchy with shorter module names #174

Merged
merged 11 commits into from
Jul 11, 2020

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Jun 30, 2020

Addresses #110.


TODO:

  • prettyprinter library
  • prettyprinter-ansi-terminal library
  • prettyprinter-convert-ansi-wl-pprint library
  • prettyprinter-compat-*
  • Tests and benchmarks
  • generate_readme etc.

Travis: https://travis-ci.org/github/quchen/prettyprinter/pull_requests

@sjakobi sjakobi marked this pull request as draft June 30, 2020 12:29
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jun 30, 2020

The best way to review this is probably simply to build the haddocks locally and browse them…

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 4, 2020

hlint makes CI fail. :(

prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs:178:17-42: Suggestion: Reduce duplication
Found:
  currentStyle <- unsafePeek
  let newStyle = style <> currentStyle
  push newStyle
  
Perhaps:
  Combine with prettyprinter-ansi-terminal/src/Prettyprinter/Render/Terminal/Internal.hs:240:17-42

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 5, 2020

I think I'll hold off on converting the rest until I'm more confident that we'll eventually remove the Data.Text.Prettyprint hierarchy.

@sjakobi sjakobi marked this pull request as ready for review July 5, 2020 12:56
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 6, 2020

-- | This module is part of the old @prettyprinter-ansi-terminal@ API which is
-- planned to be deprecated and eventually removed.
--
-- Use "Prettyprinter.Render.Terminal" instead.

I wonder whether "API" might be misunderstood to mean that the types and functions are changed too…

Maybe it would be better to say "module hierarchy" or "namespace"…

@quchen
Copy link
Owner

quchen commented Jul 8, 2020

Looks good to me. Some of the deeper modules can’t be made super short, such as Prettyprinter.Render.Terminal.Internal, but then again those don’t regularly need to be referred to by the library’s users.

As for the »API«, I think »rather deep module naming hierarchy« is proably a bit verbose, but also very descriptive.

@quchen
Copy link
Owner

quchen commented Jul 8, 2020

Timeframe-wise I think we should err on the safer side – there is no need to remove the deprecated modules for years, really. Developing the library has taught me a bit about #ifdef imports and how pleasant they are to use ;-)

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jul 11, 2020

Timeframe-wise I think we should err on the safer side – there is no need to remove the deprecated modules for years, really. Developing the library has taught me a bit about #ifdef imports and how pleasant they are to use ;-)

Indeed there's not much need to rush. The most significant downside to having both module hierarchies is that the package overview is somewhat cluttered. Nevertheless, I think we can keep up the deprecation for at least a year.

@sjakobi sjakobi merged commit f9faed5 into master Jul 11, 2020
@sjakobi sjakobi deleted the 110-shorter-module-names branch July 11, 2020 13:49
sjakobi added a commit that referenced this pull request Jul 11, 2020
@istathar
Copy link

istathar commented Aug 3, 2020

The most significant downside to having both module hierarchies is that the package overview is somewhat cluttered.

If you use

{-# OPTIONS_HADDOCK hide #-}

in a module header alongside the language pragmas it'll suppress that module from appearing in generated Haddock output. I use it to hide internal modules without making it impossible to access them. Works really well for deprecated modules, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants