Skip to content

Add docstrings and follow up on some conventions #9

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

Merged
merged 19 commits into from
Nov 24, 2017

Conversation

volrath
Copy link
Member

@volrath volrath commented Sep 23, 2017

A few things to notice:

  • In parseclj.el:
    • I moved parseclj--leaf-tokens and parseclj--closing-tokens to parseclj-lex.el, since tokens are lexical structures.
    • I moved parseclj--string, parseclj--character and parseclj--leaf-token-value to parseedn.el. Reasoning behind it is that these function produce emacs lisp values, which goes with everything else in parseedn.
    • I changed error messages. Initially I changed them because they were generating checkdoc warnings (format-message messages should start with capital letters, and I didn't think parseclj: should be capitalized.) After reading through the Signaling Errors documentation, I started thinking that maybe we should take a different approach with the way we are signaling. Maybe instead of providing a single string message, we provide a tuple of position (int) and message (str). This way is more similar to other errors like wrong-type-argument or wrong-number-of-arguments. Also, documentation says that:

If the error is not handled, the two arguments are used in printing the error message. Normally, this error message is provided by the error-message property of error-symbol.

And parseclj-parse-error's message is already "parseclj: Syntax Error". But I'm not 100% sure if this covers everything we want from an error message.

  • In parseclj-ast.el:
    • I moved unparsing functions to this file and removed parseclj-unparse. These functions are only for unparsing our AST data structure definition anyway.

@@ -103,6 +186,8 @@ handlers as an optional argument to the reader functions.")
(parseedn-print-kvs next)))))

(defun parseedn-print (datum)
"Insert DATUM as EDN into the current buffer.
DATUM can be any Emacs Lisp value."
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this true? not entirely sure. If it isn't, maybe we should add a t clause at the end that catches possible errors.

parseedn.el Outdated
(require 'parseclj-lex)

(defun parseedn--string (s)
""
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally left this one blank. Not quite sure I understand everything going on here and I thought it was better to ask you for clarification. Maybe we could add some one-line comments to each of the replace-regexp-strings as well.

(if (parseclj-lex-at-eof?)
"Consume characters at point and return the next lexical token.

See `parseclj-lex-token'."
Copy link
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 really happy with this docstring, I feel it should have a little bit more content, but I'm not sure what else to put here. thoughts?

Copy link
Member Author

@volrath volrath left a comment

Choose a reason for hiding this comment

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

Some other extra comments.

This commit also moves `leaf-token-value` to `parseclj-lex`
@volrath
Copy link
Member Author

volrath commented Sep 28, 2017

According to our conversation yesterday, I moved the shift/reduce parser to its own module. Called it parseclj-parser.el as a provisional name, just let me know if you are ok with it.

I also moved the --leaf-token-value from parseedn to parseclj-lex, where I think it fits nicely because it's a lexical token function. But it might also work in parseclj-parser.

Now parseedn can be ran standalone, only depending on parseclj-parser and a. No other library depends on parseedn either.

volrath and others added 4 commits November 15, 2017 16:10
Reduction was returning only the reduced tag without the rest of the stack, so
all previous elements were lost.
Fix `parseclj-ast--reduce-branch` for tags.
@plexus plexus merged commit 2588470 into master Nov 24, 2017
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.

2 participants