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

Do not error when there are no tests #489

Merged
merged 2 commits into from
Nov 9, 2019
Merged

Do not error when there are no tests #489

merged 2 commits into from
Nov 9, 2019

Conversation

waddlaw
Copy link
Contributor

@waddlaw waddlaw commented Nov 9, 2019

Description of the change

I'm not familiar with PureScript, so could you check it?

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

Copy link
Member

@f-f f-f 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, thank you 🙂

@f-f f-f changed the title Spago test errors when there are no tests Do not error when there are no tests Nov 9, 2019
@f-f f-f merged commit fccf87e into purescript:master Nov 9, 2019
@waddlaw waddlaw deleted the spago-test-errors-when-there-are-no-tests branch November 10, 2019 00:56
@waddlaw
Copy link
Contributor Author

waddlaw commented Nov 10, 2019

@f-f Thank you for merging!

I read the source code of the PureScript parser (Language.PureScript.CST.Parser). The current implementation is incomplete compared to the original parser. Is it better to implement everything?

I also considered using the PureScript Compiler API, but gave up because it didn't build. There are also concerns that dependencies will be heavy.

moduleHeader :: { Module () }
  : 'module' moduleName exports 'where' '\{' moduleImports
      { (Module () $1 $2 $3 $4 $6 [] []) }

moduleName :: { Name N.ModuleName }
  : UPPER {% upperToModuleName $1 }
  | QUAL_UPPER {% upperToModuleName $1 }

exports :: { Maybe (DelimitedNonEmpty (Export ())) }
  : {- empty -} { Nothing }
  | '(' sep(export, ',') ')' { Just (Wrapped $1 $2 $3) }

export :: { Export () }
  : ident { ExportValue () $1 }
  | symbol { ExportOp () $1 }
  | properName { ExportType () $1 Nothing }
  | properName dataMembers { ExportType () $1 (Just $2) }
  | 'type' symbol { ExportTypeOp () $1 $2 }
  | 'class' properName { ExportClass () $1 $2 }
  | 'kind' properName { ExportKind () $1 $2 }
  | 'module' moduleName { ExportModule () $1 $2 }

ident :: { Name Ident }
  : LOWER {% toName Ident $1 }
  | 'as' {% toName Ident $1 }
  | 'hiding' {% toName Ident $1 }
  | 'kind' {% toName Ident $1 }

symbol :: { Name (N.OpName a) }
  : SYMBOL {% toName N.OpName $1 }
  | '(..)' {% toName N.OpName $1 }

properName :: { Name (N.ProperName a) }
  : UPPER {% toName N.ProperName $1 }

dataMembers :: { (DataMembers ()) }
  : '(..)' { DataAll () $1 }
  | '(' ')' { DataEnumerated () (Wrapped $1 Nothing $2) }
  | '(' sep(properName, ',') ')' { DataEnumerated () (Wrapped $1 (Just $2) $3) }

@f-f
Copy link
Member

f-f commented Nov 10, 2019

@waddlaw great points!

I think that we should not depend on the compiler package, as I feel it's way too big. So I'd say we could port part of the parser here (as we're doing now) and eventually split off the parser from the compiler and depend on that one instead (the idea for this to happen is already there, see purescript/purescript#3633)

And yes, I'd say it's better to implement everything from the actual parser, otherwise we risk failing on otherwise good source code!

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.

spago test errors when there are no tests
2 participants