Skip to content

Separate the Op and Ident namespaces, add phantom type for Op for values/types#2090

Merged
garyb merged 3 commits intopurescript:0.9from
garyb:idents
May 11, 2016
Merged

Separate the Op and Ident namespaces, add phantom type for Op for values/types#2090
garyb merged 3 commits intopurescript:0.9from
garyb:idents

Conversation

@garyb
Copy link
Copy Markdown
Member

@garyb garyb commented May 4, 2016

And remove a few remnants of pre-0.9 operator code.

Still very much a WIP, this got a bit more involved than I expected.

@garyb garyb added this to the 0.9.0 milestone May 4, 2016
@garyb garyb self-assigned this May 4, 2016
@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 4, 2016

I had a go at this, but it was a bit beyond my understanding. Thanks!

paf31
paf31 reviewed May 4, 2016
View reviewed changes
src/Language/PureScript/Parser/Common.hs
--
parseIdent :: TokenParser Ident
parseIdent = (Ident <$> identifier) <|> (Op <$> parens symbol)
parseIdent = Ident <$> identifier
Copy link
Copy Markdown
Contributor

@paf31 paf31 May 4, 2016

Choose a reason for hiding this comment

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

👍 Much nicer.

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 4, 2016

Yeah, hopefully when this PR is done the result should be much more understandable! Since I delved into the operators lately I mostly have it loaded into my brain still.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 4, 2016

Also note there are some comments about 0.9 in the parser module. See parseVarOrNamedBinder and parseAbs for places where we can now reduce backtracking.

garyb
garyb reviewed May 4, 2016
View reviewed changes
src/Language/PureScript/AST/Declarations.hs Outdated
-- A fixity declaration for a type operator
-- (fixity data, operator name, type the operator is an alias for)
--
| TypeFixityDeclaration Fixity (OpName 'TypeOpName) (Qualified (ProperName 'TypeName))
Copy link
Copy Markdown
Member Author

@garyb garyb May 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure about this approach yet (separating the type fixities entirely). It might result in a ton of duplicated code, but on the other hand it will eliminate a bunch of cases where we have a multiplication of possibilities due to type/value split (and with 1/3rd or 2/3rds of the possibilities being impossible, depending on the situation).

I tried some stuff out with GADTs, but it quickly spiralled into madness.

@garyb garyb force-pushed the idents branch 9 times, most recently from 0539e3c to dfd390c Compare May 9, 2016 02:02
@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 9, 2016

This will finish the last items in #2012, correct?

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented May 9, 2016

What about #1511, was that implemented already?

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 9, 2016 •
edited
Loading

#1511 will need doing too, that shouldn't be too difficult to sort out though. There's also whether we're going to do anything about the syntax for imports to go along with that. Also removing multiple modules per file, which is a one-line change.

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented May 9, 2016

I guess it depends whether #2012 is intended to be a complete list of breaking changes. If it is, I think it's missing #1768 too, right?

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 9, 2016

Good point! I think it is supposed to be a comprehensive list.

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 10, 2016

This is now ready for review. It cleans up the remainder of the 0.9 stuff relating to operators, and I reduced the number of different types of errors we need by making wider use of the Name sum type, rather than it just being for the linter. Sorry the diff is so horrendous. 😉

There's a problem in the psc-ide tests on OSX though that I don't really know what the deal is:

Failures:

  tests/Language/PureScript/Ide/Imports/IntegrationSpec.hs:46: 

  1) Language.PureScript.Ide.Imports.Integration, Adding imports, adds an implicit import

       uncaught exception: IOException of type OtherError (readCreateProcess: psc-ide-client (exit 1): failed)

Any ideas @kritzcreek?

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 10, 2016

This will interfere with #2079 actually, but I think it might help: the printing of names should be much more consistent now, as I think they go through the same function. Aside from type variables and row labels.

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented May 10, 2016

From the travis log:

psc-ide-server: bind: resource busy (Address already in use)

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 10, 2016

Oh, oops, I didn't see that up there, just the failure output. That's weird.

@kritzcreek
Copy link
Copy Markdown
Member

kritzcreek commented May 10, 2016

Omw home right now, I'll check soon but it might be related to the psc-ide network PR. Most likely a server instance didn't die fast enough.

@hdgarrood
Copy link
Copy Markdown
Contributor

hdgarrood commented May 10, 2016

I guess it's because it was from the output of a subprocess (psc-ide-server)? Yeah, I was about to say, perhaps the setup and teardown functions should use waitForProcess to ensure the next test doesn't start prematurely: http://hackage.haskell.org/package/process-1.4.2.0/docs/System-Process.html#v:waitForProcess

@kika @garyb
Fixes purescript#2064 (purescript#2107)
e93bfbf 
* Fixes purescript#2064

The unnecessary forward (client) and reverse (server) name resolution
was impaired on OS X when IPv6 entries for localhost were missing in
/etc/hosts
The proposed patch remove this name resolution and makes psc-ide
operation independent from name resolution proper configuration.

* Added @kika to CONTRIBUTORS.md
@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 10, 2016

No joy. Will have to try the waitForProcess approach. Strange that this doesn't break elsewhere though.

@coveralls
Copy link
Copy Markdown

coveralls commented May 10, 2016

Coverage Status

Coverage decreased (-0.2%) to 56.048% when pulling c19a1a0 on garyb:idents into e11cff5 on purescript:0.9.

@garyb
Copy link
Copy Markdown
Member Author

garyb commented May 10, 2016

There we go. Seems quitting the server before the tests started was causing the problem for some reason.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 11, 2016 •
edited
Loading

🚢 it!

@garyb garyb merged commit 3ad48f1 into purescript:0.9 May 11, 2016
@garyb garyb deleted the idents branch May 11, 2016 11:27
@garyb garyb mentioned this pull request May 17, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@garyb garyb

Labels

status: ready for review

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

6 participants

@garyb @paf31 @hdgarrood @kritzcreek @coveralls @kika