-
Notifications
You must be signed in to change notification settings - Fork 2
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 src/colis.ml
into a library and an executable
#30
Conversation
Niols
commented
Oct 16, 2018
- It makes the separation of code cleaner
- It allows for an easy use of internal functions (eg for testing purpose)
- It allows for a better documentation of internal functions
- It will allow an other tool to use the internal library directly instead of using the executable
src/colis_cmd.ml
Outdated
eprintf "Parsing error@."; | ||
exit 2 | ||
| Morsmall.SyntaxError _pos -> | ||
eprintf "Syntax error@."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about showing the same error message for syntax errors in Shell (Morsmall.SyntaxError
) and Colis (Colis.ColisParser
)? (BTW it would be nice if we could have a pos
for the Colis parser as well and display it. I’ll have a look later…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. If fact, in the lib Colis
, we could have only one exception for all lexing/parsing errors, what do you think? And indeed, it would be nice to carry a position in the parser (which reminds me of #19: do we want positions in the AST?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only one exception for all lexing/parsing errors
Yes good idea.
It will certainly be interesting to be able to point to the position in the shell script from within the symbolic execution. But IMO we can postpone adding positions to the AST until we are there – it will require refactoring but won’t be technically complicated. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only one exception for all lexing/parsing errors
Yes good idea.
OK, commit inc
It will certainly be interesting to be able to point to the position in the shell script from within the symbolic execution. But IMO we can postpone adding positions to the AST until we are there – it will require refactoring but won’t be technically complicated. What do you think?
So at some point we will refactor both the AST, the conversion and the parser to add positions everywhere? Fine with me
Very useful PR – I was already alienated by how much logic should be put into the |
OK looks good to me! |
Do you think we should merge it? |
Definitely |