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 PrintOptions for pretty printer. #100

Merged
merged 4 commits into from
Oct 4, 2018
Merged

Add PrintOptions for pretty printer. #100

merged 4 commits into from
Oct 4, 2018

Conversation

astynax
Copy link
Contributor

@astynax astynax commented Oct 2, 2018

Closes #11

@chshersh chshersh added pretty-printer Everything related to `Toml -> Text` Hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 3, 2018
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change! We're almost there, I have only couple comments

prettyTables :: Int -> Text -> PrefixMap TOML -> Text
prettyTables i pref = Text.concat . map prettyTable . HashMap.elems
prettyTables :: PrintOptions -> Int -> Text -> PrefixMap TOML -> Text
prettyTables options i pref = Text.concat . map prettyTable . HashMap.elems
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's a good idea to apply order here as well. I think if you specify shouldSort, you should sort table names as well.

{- | Default printing options. -}
defaultOptions :: PrintOptions
defaultOptions = PrintOptions True 2

-- Tab is equal to 2 spaces now.
tab :: Int -> Text
tab n = Text.cons '\n' (Text.replicate (2*n) " ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also patch this function and replace 2 * n with n. As I understand correctly, the tab function will be given numbers 0, 2, 4, 6, 8, ... or 0, 4, 8, 12, ... (depending on the indentation size) and previously it took numbers 0, 1, 2, 3, .... So instead of multiplying sequent natural numbers by indentation we can just add indent to the nextI (as you already did) but also remove multiplier from the tab function

@chshersh
Copy link
Contributor

chshersh commented Oct 3, 2018

The CI error is really weird and I don't quite understand, why it happens... Requires further investigation.

Also, could you please update the CHANGELOG.md?

@vrom911
Copy link
Member

vrom911 commented Oct 3, 2018

CI is fixed now! See the comment in the other tomland PR: #101 (comment)

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring! There's only one bug in the current implementation.

Also, could you please update the CHANGELOG.md with the short description of your changes?

After that, I think, this PR can be merged 👍

-- Returns an indentation prefix
tabWith :: PrintOptions -> Int -> Text
tabWith options n =
Text.cons '\n' (Text.replicate (n * indent options) " ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n argument is redundant here and there's no need to multiply by n. In the code you assign nextI variable (which is responsible for indentation) to the value i + indent options and you pass options and i to the tab function. So, for example, if default indentation is 4 then layers will have the following indents:

  • Nested 0: indent = 4, n = 0 => tab is 0 spaces
  • Nested 1: indent = 4, n = 4 => tab is 16 spaces
  • Nested 2: indent = 4, n = 8 => tab is 32 spaces
  • and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought about it, but forgot to drop + indent options...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now n means only a level of indentation (as it was before)

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition to the library! Thank you very much, @astynax ! ❤️

@chshersh chshersh merged commit 3187fed into kowainik:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest https://hacktoberfest.digitalocean.com/ pretty-printer Everything related to `Toml -> Text`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants