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

[#39] String interpolation in coffer output messages #63

Merged
merged 1 commit into from
May 5, 2022

Conversation

DK318
Copy link
Member

@DK318 DK318 commented Apr 5, 2022

Description

Problem

At this moment constructing some messages in Main.hs
looks very convoluted (e.g. message in set-field command).

Solution

Used nyan-interpolation package in Main.hs.

Related issue(s)

✅ Checklist for your Pull Request

Related changes (conditional)

  • Tests
    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation
    • I checked whether I should update the docs and did so if necessary:
  • Public contracts
    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

Copy link
Member

@dcastro dcastro left a comment

Choose a reason for hiding this comment

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

LGTM, just one small suggestion! 👍

app/Main.hs Outdated
Comment on lines 228 to 231
let entryTo = getEntryFromCreateError err
let header = "'" +| from |+ "' to '" +| entryTo ^. path |+ "':"
let header = [int|s|'#{from}' to '#{entryTo ^. path}':|]
let errorMsg = createErrorToBuilder err
pure $ unlinesF @_ @Builder $ header : [indentF 2 errorMsg]
Copy link
Member

Choose a reason for hiding this comment

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

I didn't test it, but would this work?

          let entryTo = getEntryFromCreateError err
          pure [int||
            '#{from}' to '#{entryTo ^. path}':
              #{createErrorToBuilder err}
          |]

Copy link
Member

Choose a reason for hiding this comment

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

Actually, also just noticed that buildErrorMessages doesn't have to return Sem r [Builder], it could just return a [Builder].

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I'm afraid no. Just a couple of days ago I witnessed the need for such functionality too, so let the ticket be serokell/nyan-interpolation#11.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, and one of the messages returned by createErrorToBuilder does have multiple lines :/

Ok, let's leave it like this for now. @DK318, please create a separate issue to improve this when serokell/nyan-interpolation#11 is done + released.

Copy link
Member

Choose a reason for hiding this comment

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

Note: while we're here, we could still change Sem r [Builder] -> [Builder] in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DK318 I checked, Sem r [Builder] can be changed to [Builder] (as mentioned above). For example, like this:

buildErrorMessages = fmap \(from, err) ->
  let entryPath = getEntryFromCreateError err
      header    = [int|s|'#{from}' to '#{entryTo ^. path}':|]
      errorMsg  = createErrorToBuilder err
  in unlinesF @_ @Builder $ header : [indentF 2 errorMsg]

@dcastro dcastro requested a review from sancho20021 May 3, 2022 12:25
Copy link
Contributor

@sancho20021 sancho20021 left a comment

Choose a reason for hiding this comment

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

Please, resolve conflicts with the main branch and maybe change the buildErrorMessages type :)

Everything else looks good

app/Main.hs Outdated
Comment on lines 228 to 231
let entryTo = getEntryFromCreateError err
let header = "'" +| from |+ "' to '" +| entryTo ^. path |+ "':"
let header = [int|s|'#{from}' to '#{entryTo ^. path}':|]
let errorMsg = createErrorToBuilder err
pure $ unlinesF @_ @Builder $ header : [indentF 2 errorMsg]
Copy link
Contributor

Choose a reason for hiding this comment

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

@DK318 I checked, Sem r [Builder] can be changed to [Builder] (as mentioned above). For example, like this:

buildErrorMessages = fmap \(from, err) ->
  let entryPath = getEntryFromCreateError err
      header    = [int|s|'#{from}' to '#{entryTo ^. path}':|]
      errorMsg  = createErrorToBuilder err
  in unlinesF @_ @Builder $ header : [indentF 2 errorMsg]

@DK318
Copy link
Member Author

DK318 commented May 3, 2022

Oh, this PR is really outdated. Still waiting for a review 😄

@dcastro
Copy link
Member

dcastro commented May 5, 2022

Oh, this PR is really outdated. Still waiting for a review 😄

Looks like you have 2 reviews now, I really only had that 1 small comment ^^

@DK318
Copy link
Member Author

DK318 commented May 5, 2022

Oh. This Review required got me distracted

@DK318 DK318 force-pushed the dk318/#39-strings-interpolation branch from 5432993 to ce9d09a Compare May 5, 2022 10:46
app/Main.hs Outdated Show resolved Hide resolved
.hlint.yaml Show resolved Hide resolved
Problem: at this moment constructing some messages in `Main.hs`
looks very convoluted (e.g. message in `set-field` command).

Solution: used `nyan-interpolation` package in `Main.hs`.
@DK318 DK318 force-pushed the dk318/#39-strings-interpolation branch from f9505cf to 0608ae9 Compare May 5, 2022 16:30
@DK318 DK318 merged commit 98619bc into main May 5, 2022
@DK318 DK318 deleted the dk318/#39-strings-interpolation branch May 5, 2022 16:32
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.

Use nyan-interpolation
4 participants