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

WIP: Implement as Location import syntax #449

Closed
wants to merge 4 commits into from

Conversation

kirelagin
Copy link

@kirelagin kirelagin commented Jun 9, 2018

A fix for dhall-lang/dhall-lang#71.

This PR adds two new types:

  • FilePath
  • Url

and literals for them. The syntax is

  • ./some/path as Location and
  • https://example.com/url as Location respectively.

Trying to use an env: import with as Location is an error.

TODO:

  • Update the language spec :).
  • Take headers which come from hashes (?) into account in diff.
  • Use a real Uri type for Url literals instead of current Text?

@kirelagin
Copy link
Author

Take headers which come from hashes (?) into account in diff.

We agreed that it does not make sense to specify a hash for a url used as Location, so we should throw an error. (Another reasonable option would be to ignore it completely, which is how it works at the moment.)

@kirelagin
Copy link
Author

It turns out, I overlooked one part of url syntax, namely, using for specifying request headers. I think, the right thing to do is to prohibit them in url values.

I have actually implemented it already, will push later because no wifi.

@kirelagin
Copy link
Author

I am not really sure what to do with urls. I am now starting to think that adding a dependency on some library for handling urls might be not worth it. What do you think?

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Looks great! I can help with the matching change to the standard (hopefully tomorrow)

@@ -352,6 +357,14 @@ data Expr s a
| TextLit (Chunks s a)
-- | > TextAppend x y ~ x ++ y
| TextAppend (Expr s a) (Expr s a)
-- | > FilePath ~ FilePath
| FilePath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps shorten the name FilePath to File

Copy link
Author

Choose a reason for hiding this comment

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

Well, I don’t know, are you sure? I feel that it will be somewhat more ambiguous. Also, currently it nicely corresponds to the Haskell type, as other primitive types (e.g. Integer, Text).

buildExprF FilePath =
"FilePath"
buildExprF Url =
"Url"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps Url should be capitalized as URL since it is an acronym

Copy link
Author

Choose a reason for hiding this comment

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

Oh, sorry, I always capitalise this way, as most coding guidelines suggest using camel case with acronyms, so I didn’t even think about it.

Anyway, I can capitalise the text representation of the type, but if I change the constructor itself, then it will conflict with URL from ImportType. I guess, I can add Import as a prefix to constructors of ImportType. What do you think?

URL prefix file suffix maybeHeaders -> do
m <- needManager
instance Show IllegalImportType where
show EnvAsLocation = _ERROR <> ": env cannot be imported as a location"
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/env/an environment variable/ and s/as location/❰as Location❱/

Copy link
Author

Choose a reason for hiding this comment

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

mismatch l r
diffExprF l r@(FilePathLit _) =
mismatch l r
-- TODO: Diff maybeHeaders?
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we discussed offline, we can probably have import resolution fail if the user specifies both using and as Location

@Gabriella439
Copy link
Collaborator

Oh, one other thing: don't forget to mention the as Location feature somewhere in the tutorial

@kirelagin
Copy link
Author

By the way, I finally read some code around the parts I changed (😄) and I should point out that new relative path literals (somewhat) violate referential transparency: the check that local files cannot be imported from a remote one is done only when actually reading, so it is not performed for as Location.

On the one hand, it makes perfect sense for the cases when, e.g. some sort of default configuration is loaded from a remote URL and paths to other files in it are resolved relative to current directory and it probably doesn’t harm too much, since the referred files are not interpreted and even not read. On the other hand, your notion of referential transparency seems to be a good idea. I’m not sure what to do.

@kirelagin
Copy link
Author

Wait, no, it is resolved relative to the URL of the file that contains it, not to current directory 😕.
Status update: I have no idea how it works...

@Gabriella439
Copy link
Collaborator

@kirelagin: What use case do you have in mind for this? I ask because in dhall-lang/dhall-lang#71 one of the ideas was that the import would actually be checked to see if it was valid

The reason I mention this is that if you validate the import tagged with as Location then the hash and headers become relevant again and the referential transparency check gets invoked

@kirelagin
Copy link
Author

Well, I have a felling (although, I have no evidence to back this claim) that software typically wants to just read a path from its config and then deal with any issues itself (e.g. if a database does not exist, create it) instead of getting an exception from its configuration “parser”.

@Gabriella439
Copy link
Collaborator

@kirelagin: Then let's leave it as is for now, but with one change: don't reject custom headers or a semantic integrity check. The idea is that maybe later we'll add a as Valid Location extension that will actually check if the location exists and is importable

@ocharles
Copy link
Member

ocharles commented Jul 1, 2018

Is there just @Gabriel439's last comment to address before this can get merged? I'm really keen to have this feature.

@kirelagin
Copy link
Author

I’m sorry, I got very distracted from this PR.
As I understand it, the only thing that needs to be done in this PR is to undo the checks @Gabriel439 mentioned. But before this PR can be merged, a description of the behaviour implemented here must be added to the standard.

I can fix this PR hopefully in the next few days, but I am not sure I will have time to update the standard, so any help is really welcome.

@Gabriella439
Copy link
Collaborator

Yeah, the standard is the big issue. This has to be standardized before the Haskell implementation can be changed

You don't necessarily need to put up a pull request with the changes to the semantics and grammar. Just open an issue against the dhall-lang repository with an informal description of the proposed change so that people can discuss it and the motivation behind it

@Gabriella439
Copy link
Collaborator

I'm closing this since it's drifted quite a bit from master and this still needs to be standardized before implementation

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.

None yet

3 participants