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

Integration of dhall #72

Merged
merged 46 commits into from
Sep 4, 2018
Merged

Integration of dhall #72

merged 46 commits into from
Sep 4, 2018

Conversation

jneira
Copy link
Collaborator

@jneira jneira commented Sep 4, 2018

@jneira jneira requested a review from rahulmutt September 4, 2018 07:05
Copy link
Member

@rahulmutt rahulmutt left a comment

Choose a reason for hiding this comment

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

Great work @jneira! That was an intense refactor :)

component = P.munch1 (\c -> isAlphaNum c || c `elem` "_")
parsec' = (:) <$> lead <*> rest
lead = P.satisfy (\c -> isAlphaNum c || c == '_')
rest = P.munch (\c -> isAlphaNum c || c == '_' || c == '-')
Copy link
Member

Choose a reason for hiding this comment

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

Do these changes change the parsing behavior or is it just a refactoring?

Copy link
Member

Choose a reason for hiding this comment

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

Ok looks like a refactor.

Copy link
Collaborator Author

@jneira jneira Sep 4, 2018

Choose a reason for hiding this comment

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

It is weird but testing with .cabal files gave me an error when indexing cassava with PARSEC option. It has an infamous flag with an internal double dash. Dont know why previous etlas version that has PARSEC enabled didnt throw the error
(See 1847005)

Copy link
Member

Choose a reason for hiding this comment

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

This is a weird thing in that stack.yaml in this repo turns on the parsec flag, while the stack.yaml in the main eta repo doesn't. So the etlas you use normally when doing ./cleaninstall.sh doesn't use parsec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohhh, ok, however i think i've used etlas installed from its own repo and i dont remember having that error (cassava is always indexed?).. anyway afaik the new flag parser is more robust cause it will support previous and future flag formats

Copy link
Member

Choose a reason for hiding this comment

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

Great work! :)

Nothing -> error $ "Couldn't read cabal file "
++ show fileName
parsed = if takeExtension fileName == ".dhall"
then fmap Just $ PackageDesc.Parse.parseGenericPackageDescriptionFromDhall fileName
Copy link
Member

Choose a reason for hiding this comment

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

Does parsing dhall files always succeed?

Copy link
Member

Choose a reason for hiding this comment

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

Nvm this is parsing from the index, so only valid dhall files will be parsed.

@rahulmutt rahulmutt merged commit e9b0966 into typelead:master Sep 4, 2018
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.

2 participants