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

Report comments via AST #417

Merged
merged 9 commits into from
Mar 28, 2023
Merged

Report comments via AST #417

merged 9 commits into from
Mar 28, 2023

Conversation

rkaminsk
Copy link
Member

@rkaminsk rkaminsk commented Mar 25, 2023

  • add comment ASTs to C API
  • add comment ASTs to C++ API + tests
  • add comment ASTs to Python API + tests
  • it is unnecessary to extract comments in clingo mode
  • do not print newline in astv2_str
  • ensure proper order of comment ASTs (addressing the issue below)
  • think about option to ignore comment ASTs
    • To add optional comment parsing, I would have to break the API. Most applications are written to handle a known set of AST types. Thus, adding a new AST type should not break many applications. Furthermore, passing comments should not incur much overhead. So it seems a reasonable approach to unconditionally report comment ASTs.

The snippet below shows some issues that should still (or not) be addressed before merging:

>>> from clingo.ast import parse_string
>>> 
>>> PRG = """
... %* This program shows some issues that appear
...  * when directives with optional attributes are used.
...  * Changing this would require storing comments in the lexer.
...  *%
... #const x=10.
... % comment appears unexpectedly before `x=10`
... a.
... % comment appears before `y=10` as expected
... #const y=10. [override]
... b.
... #external a.
... % comment appears unexpectedly before `#external a`
... a.
... % comment appears before `#external b` as expected
... #external b. [true]
... """
>>> 
>>> parse_string(PRG, print)
#program base.
%* This program shows some issues that appear
 * when directives with optional attributes are used.
 * Changing this would require storing comments in the lexer.
 *%
% comment appears unexpectedly before `x=10`
#const x = 10.
a.
% comment appears before `y=10` as expected
#const y = 10. [override]
b.
% comment appears unexpectedly before `#external a`
#external a. [false]
a.
% comment appears before `#external b` as expected
#external b. [true]

@rkaminsk rkaminsk added this to the v5.7.0 milestone Mar 25, 2023
@rkaminsk rkaminsk self-assigned this Mar 25, 2023
@rkaminsk rkaminsk linked an issue Mar 25, 2023 that may be closed by this pull request
@rkaminsk
Copy link
Member Author

@teiesti, could you have a look if this suits your application?

@teiesti
Copy link

teiesti commented Mar 25, 2023

I am going to have a look.

@teiesti
Copy link

teiesti commented Mar 27, 2023

I was able to make the following work on my machine which looks very promising.

>>> parse_string("%comment\nfact.", lambda ast: print(repr(ast)))
ast.Program(Location(begin=Position(filename='<string>', line=1, column=1), end=Position(filename='<string>', line=1, column=1)), 'base', [])
ast.Comment(Location(begin=Position(filename='<string>', line=1, column=1), end=Position(filename='<string>', line=1, column=9)), '%comment', 0)
ast.Rule(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=6)), ast.Literal(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=5)), 0, ast.SymbolicAtom(ast.Function(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=5)), 'fact', [], 0))), [])

I can't wait to see this being stabilized. Thanks for the effort!

Anyhow, I would like to see the pdoc documentation. Is there an easy way to generate it?

@susuhahnml You may also want to look through this!

@rkaminsk
Copy link
Member Author

I was able to make the following work on my machine which looks very promising.

>>> parse_string("%comment\nfact.", lambda ast: print(repr(ast)))
ast.Program(Location(begin=Position(filename='<string>', line=1, column=1), end=Position(filename='<string>', line=1, column=1)), 'base', [])
ast.Comment(Location(begin=Position(filename='<string>', line=1, column=1), end=Position(filename='<string>', line=1, column=9)), '%comment', 0)
ast.Rule(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=6)), ast.Literal(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=5)), 0, ast.SymbolicAtom(ast.Function(Location(begin=Position(filename='<string>', line=2, column=1), end=Position(filename='<string>', line=2, column=5)), 'fact', [], 0))), [])

I can't wait to see this being stabilized. Thanks for the effort!

Anyhow, I would like to see the pdoc documentation. Is there an easy way to generate it?

https://pdoc3.github.io/pdoc/doc/pdoc/#command-line-interface

There is not much to document though. It is just one additional type added to the AST.

@rkaminsk rkaminsk marked this pull request as ready for review March 27, 2023 20:12
@rkaminsk
Copy link
Member Author

@teiesti, @susuhahnml, this should be ready. You can already try using the development packages on ubuntu/anaconda/pypi test. -R

@rkaminsk rkaminsk merged commit e1fced9 into wip Mar 28, 2023
@rkaminsk rkaminsk deleted the feature/ast-comments branch March 28, 2023 14:56
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Optionally) provide comments as AST nodes.
2 participants