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

Split Source and Package Parser (Part 2) #113

Merged
merged 1 commit into from
May 24, 2020

Conversation

fabianhjr
Copy link
Contributor

@fabianhjr fabianhjr commented May 23, 2020

This has two commits, the first one refactors SourceToken a bit and the second one implements an (unpolished) Package Lexer/Parser and refactors Idris/Package.idr to use this new Lexer/Parser.

Feedback is appreciated. First part is #89.

One pro is that package names can now have dashes in their name UwU

Cleanup of SourceToken

There are many organizational and consistency changes, one of the more important ones being Literal being rename StringLit to correspond with CharLit, IntegerLit, DoubleLit and extracting more common code to Parser/Lexer/Common.

Change to ParseError tok and ParseError

ParseError tok was renamed ParsingError tok and ParseError was refactored to ParseError tok to accept a token type.

src/Parser/Lexer/Common.idr Outdated Show resolved Hide resolved
src/Parser/Rule/Package.idr Outdated Show resolved Hide resolved
@fabianhjr
Copy link
Contributor Author

Hi @ziman, thanks for the feedback, those two points have been added to this merge request.

@ziman
Copy link
Collaborator

ziman commented May 24, 2020

Great, thanks! I've got some more questions, I'll put them as comments in the diff.

Copy link
Collaborator

@ziman ziman left a comment

Choose a reason for hiding this comment

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

Somehow I can't see why the lexer works, but the CI symbol is green so I'm probably missing something.

src/Parser/Lexer/Common.idr Outdated Show resolved Hide resolved
src/Parser/Lexer/Common.idr Outdated Show resolved Hide resolved
@fabianhjr fabianhjr force-pushed the split-parser-2 branch 8 times, most recently from d9ee738 to e0f7e6f Compare May 24, 2020 19:23
@fabianhjr
Copy link
Contributor Author

Hey @ziman, I think this is way cleaner now, thanks for the feedback.

@fabianhjr fabianhjr force-pushed the split-parser-2 branch 4 times, most recently from d8e3138 to d5aad58 Compare May 24, 2020 20:30
Copy link
Collaborator

@ziman ziman left a comment

Choose a reason for hiding this comment

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

I've got just a few naming suggestions, and maybe a proposal to merge Ident and DotSepIdent.

Otherwise I think it's good to go.

src/Idris/Package.idr Outdated Show resolved Hide resolved
src/Idris/Package.idr Show resolved Hide resolved
src/Idris/Package.idr Show resolved Hide resolved
src/Parser/Lexer/Common.idr Show resolved Hide resolved
src/Parser/Lexer/Package.idr Outdated Show resolved Hide resolved
src/Parser/Lexer/Package.idr Outdated Show resolved Hide resolved
src/Parser/Lexer/Source.idr Show resolved Hide resolved
src/Parser/Lexer/Source.idr Outdated Show resolved Hide resolved
src/Parser/Rule/Package.idr Show resolved Hide resolved
@fabianhjr fabianhjr force-pushed the split-parser-2 branch 2 times, most recently from 38c1cce to aa64ede Compare May 24, 2020 20:57
…actor Idris/Package

Co-authored-by: Matus Tejiscak <ziman@functor.sk>
@ziman ziman merged commit 730726c into idris-lang:master May 24, 2020
@fabianhjr fabianhjr deleted the split-parser-2 branch May 24, 2020 21:33
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

2 participants