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

[refactor] Use multiline strings for better readability #1142

Closed
wants to merge 1 commit into from

Conversation

ska80
Copy link
Contributor

@ska80 ska80 commented Mar 1, 2021

No description provided.

@melted
Copy link
Collaborator

melted commented Mar 1, 2021

There hasn't been a release with multiline strings, so it's too early to introduce them in the compiler as it will break the build with the last release.

@melted
Copy link
Collaborator

melted commented Mar 1, 2021

Where is the CI?

@ska80
Copy link
Contributor Author

ska80 commented Mar 1, 2021

Where is the CI?

No idea why it can't run. It just shows "Queued" status.

@ska80
Copy link
Contributor Author

ska80 commented Mar 1, 2021

There hasn't been a release with multiline strings, so it's too early to introduce them in the compiler as it will break the build with the last release.

I guess this can wait and then I will rebase when a release comes out.

@ska80
Copy link
Contributor Author

ska80 commented Mar 1, 2021

@andrevidela
Copy link
Collaborator

As much as everyone is excited about multi-line strings I would hold off on using them in the compiler, there likely will be breaking changes soon-ish, since not all features have been implemented yet. Most notably the changes to typechecking have not been implemented and that is:

  • automatically infer show calls
  • manually control run-time behaviour through linearity annotations

Those are tracked here #555
There are some rough edges with error reporting too! see #1133 #1132

@buzden
Copy link
Contributor

buzden commented Mar 1, 2021

By the way, when suggesting such a change, why not to indent the whole multiline literal since it's supported too?
I mean, instead of writing

Show Metadata where
  show (MkMetadata apps names tydecls currentLHS holeLHS) = #"""
Metadata:
 lhsApps: \#{ show apps }
 names: \#{ show names }
 type declarations: \#{ show tydecls }
 current LHS: \#{ show currentLHS }
 holes: \#{ show holeLHS }
"""#

as in the PR, to write it like

Show Metadata where
  show (MkMetadata apps names tydecls currentLHS holeLHS) = #"""
    Metadata:
     lhsApps: \#{ show apps }
     names: \#{ show names }
     type declarations: \#{ show tydecls }
     current LHS: \#{ show currentLHS }
     holes: \#{ show holeLHS }
    """#

or even

Show Metadata where
  show (MkMetadata apps names tydecls currentLHS holeLHS) = """
    Metadata:
     lhsApps: \{ show apps }
     names: \{ show names }
     type declarations: \{ show tydecls }
     current LHS: \{ show currentLHS }
     holes: \{ show holeLHS }
    """

which all the same, but indented versions read better.

src/Compiler/ES/Javascript.idr Show resolved Hide resolved
src/Compiler/RefC/RefC.idr Outdated Show resolved Hide resolved
@gallais gallais added this to the 0.5.0 milestone Mar 9, 2021
@ska80 ska80 force-pushed the use-multiline-strings branch 2 times, most recently from 4194966 to 43413f9 Compare March 11, 2021 11:34
@andylokandy
Copy link
Contributor

I think it's no longer be blocked since 0.4 has just been released.

@ska80
Copy link
Contributor Author

ska80 commented Jun 25, 2021

I think it's no longer be blocked since 0.4 has just been released.

Synced with master.

src/Compiler/Scheme/Chez.idr Show resolved Hide resolved
\{
unlines [" (load-shared-object \"" ++ escapeStringChez lib ++ "\")" | lib <- libs]
})
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""
"""

Comment on lines +87 to +103
startChezWinSh chez appDirSh targetSh = #"""
#!/bin/sh

set -e # exit on any error

DIR=$(dirname "$(readlink -f -- "$0")")
CHEZ=$(cygpath "\#{ chez }")

export PATH="$DIR/\#{ appDirSh }":$PATH

"$CHEZ" --program "$DIR/\#{ targetSh }" "$@"

"$CHEZ" -q \
--libdirs "$DIR/\#{ appDirSh }" \
--program "$DIR/\#{ targetSh }" \
"$@"
"""#
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, the hashtag escape is not necessary because the quote in the multiline string is not considered as an end symbol.

@edwinb
Copy link
Collaborator

edwinb commented Jul 15, 2021

This is marked "blocked" and "do not merge" and has a few conflicts. It loooks like a nice refactoring, but it does seem to have got stale, so I'm going to close it for the sake of tidiness. If that's premature, please feel free to reopen (but also please try to resolve the blockage before doing so!)

@edwinb edwinb closed this Jul 15, 2021
@andylokandy
Copy link
Contributor

andylokandy commented Sep 22, 2021

Is it not blocked anymore since 0.5 is released?

@ohad
Copy link
Collaborator

ohad commented Sep 22, 2021

I agree with @andylokandy : the reason it was marked blocked was

  1. we wanted multiline strings (+ perhaps interpolation) to make it into the previous release for bootstrapping purposes here.
  2. some aspects of these might change (@andrevidela 's comment):
    a. automatically infer show calls
    b. some interaction with linearity annotations
    c. issues with error reporting.

I think (1) is now fine, since we've had multiline strings in the previous release (I think?).
I'm not sure why (2) blocks, since these would be backwards compatible changes

@ska80
Copy link
Contributor Author

ska80 commented Oct 22, 2021

I agree with @andylokandy : the reason it was marked blocked was

1. we wanted multiline strings (+ perhaps interpolation) to make it into the previous release for bootstrapping purposes [here](https://github.com/idris-lang/Idris2/pull/1142#issuecomment-787855099).

2. some aspects of these might change (@andrevidela 's [comment](https://github.com/idris-lang/Idris2/pull/1142#issuecomment-787951374)):
   a. automatically infer show calls
   b. some interaction with linearity annotations
   c. issues with error reporting.

Given that generic interpolation has already been merged, I created a new PR #2044 because I couldn't reopen this one.

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

Successfully merging this pull request may close these issues.

8 participants