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.

Sign up for GitHub

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

Jump to bottom

Add Token_Stream class as new lexer and ast_link to every token #61

Merged
merged 1 commit into from
Mar 3, 2024
Merged

Add Token_Stream class as new lexer and ast_link to every token #61

merged 1 commit into from
Mar 3, 2024

Conversation

markusrosskopf
Copy link
Contributor

@markusrosskopf markusrosskopf commented Jan 13, 2024 •
edited
Loading

Infos:

  • I will take out the assertion for the ast_link attribute at the end of your review. I will squash the commits.
  • I have not considered Markup-Strings yet. They are just normal Strings.
  • I added string representations for some ast objects to better see how things are connected.
  • All ast_links are set in the parser except for import statements as their names are resolved in the ast.py.
  • The import token itself is set to the files own package because it was easier to set the link at the moment the token first appears. The names are resolved after the graph was built and set correctly to their own ast package's object.
  • Comments are linked to the files main package. I have put that at the end of each file's parsing because comments could appear at the top before the "package ..." clause.

@markusrosskopf markusrosskopf force-pushed the feature/add-token-stream-with-ast-links branch from 81060aa to 48eb6d2 Compare January 17, 2024 11:02
@phiwuu phiwuu requested a review from florianschanda January 17, 2024 11:09
phiwuu
phiwuu requested changes Jan 17, 2024
View reviewed changes
trlc/trlc.py Outdated
Comment on lines 790 to 798
# for file, parser in sm.all_files.items():
# for tok in parser.lexer.tokens:
# if tok.value is None:
# token = tok.kind
# else:
# token = tok.value
# print(f"Token: {token} => {tok.ast_link}")
# assert tok.ast_link is not None, \
# f"Token {tok} in {file} has no ast_link"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phiwuu: Thanks for your feedback! It might be better to wait. Reviewing the pull request could become more challenging without these lines. I've referenced them in the pull request description.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove now, and merge afterwards.

@florianschanda
Copy link
Collaborator

Hey, I was a bit absent for a while but I will have a look at this soon!

phiwuu
phiwuu previously approved these changes Feb 19, 2024
View reviewed changes
@markusrosskopf markusrosskopf force-pushed the feature/add-token-stream-with-ast-links branch from 48eb6d2 to 6b78960 Compare March 1, 2024 07:56
@phiwuu phiwuu added the topic: core Affects lexer/parser/infrastructure label Mar 3, 2024
@phiwuu phiwuu merged commit 26645ea into bmw-software-engineering:main Mar 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phiwuu phiwuu phiwuu approved these changes

@florianschanda florianschanda Awaiting requested review from florianschanda

Assignees
No one assigned
Labels
topic: core Affects lexer/parser/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markusrosskopf @florianschanda @phiwuu