Skip to content

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

Merged
garyb merged 3 commits into
purescript: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 into
purescript:0.9from
garyb:idents

Conversation

@garyb

@garyb garyb commented May 4, 2016

Copy link
Copy Markdown
Member

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

paf31 commented May 4, 2016

Copy link
Copy Markdown
Contributor

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

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

@paf31 paf31 May 4, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Much nicer.

@garyb

garyb commented May 4, 2016

Copy link
Copy Markdown
Member Author

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

paf31 commented May 4, 2016

Copy link
Copy Markdown
Contributor

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
Comment thread 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))

@garyb garyb May 4, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

paf31 commented May 9, 2016

Copy link
Copy Markdown
Contributor

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

@hdgarrood

hdgarrood commented May 9, 2016

Copy link
Copy Markdown
Contributor

What about #1511, was that implemented already?

@garyb

garyb commented May 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

#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

hdgarrood commented May 9, 2016

Copy link
Copy Markdown
Contributor

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

garyb commented May 9, 2016

Copy link
Copy Markdown
Member Author

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

@garyb

garyb commented May 10, 2016

Copy link
Copy Markdown
Member Author

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

garyb commented May 10, 2016

Copy link
Copy Markdown
Member Author

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

hdgarrood commented May 10, 2016

Copy link
Copy Markdown
Contributor

From the travis log:

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

@garyb

garyb commented May 10, 2016

Copy link
Copy Markdown
Member Author

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

@kritzcreek

kritzcreek commented May 10, 2016

Copy link
Copy Markdown
Member

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

hdgarrood commented May 10, 2016

Copy link
Copy Markdown
Contributor

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

garyb commented May 10, 2016

Copy link
Copy Markdown
Member Author

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

@coveralls

coveralls commented May 10, 2016

Copy link
Copy Markdown

Coverage Status

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

@garyb

garyb commented May 10, 2016

Copy link
Copy Markdown
Member Author

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

@paf31

paf31 commented May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

🚢 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