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

Fixed issue 231 #240

Closed
wants to merge 1 commit into from
Closed

Conversation

MSattrtand
Copy link
Collaborator

@MSattrtand MSattrtand commented Jul 4, 2024

Fixes issue #231.

image
If an incorrect query causes an exception, the query is logged now.

@blcham
Copy link
Contributor

blcham commented Jul 4, 2024

  1. for referencing issues use # ... as it is very well integrated with github + use more specific commits:
    Fixed issues 231 ---> [#231] Improve logging for queries when parse error occurs
  2. line 32 is not telling me where to look. I think you logged a shorter query without prefixes. BTW, this was the main motivation for this issue.

@MSattrtand
Copy link
Collaborator Author

The stack trace shows that the exception has occurred at the perform function of the ParserARQ class right here:
image
The last of the s-pipes functions that may cause the exception are ApplyConstructModule and BindBySelectModule, so if we assume that foreign libraries are working correctly, the main reason the parser may fail is that our query is truly incorrect. Therefore, I thought logging only the query and the stack trace was enough for debugging.
NB: There are more functions that may (theoretically) send incorrect queries to the parser; ApplyConstructModule and BindBySelectModule are the only modules I can properly test because they are called during the execution of the Hello, World script.

@blcham
Copy link
Contributor

blcham commented Jul 5, 2024

The stack trace shows that the exception has occurred at the perform function of the Parse .....

Not sure what you mean, but I am saying that the error you are logging is not good enough because you won't be able to find the error within the query. Try to read what you sent me above and find the error based on the coordinates: in you output:
... at line 32, column 7

@blcham blcham self-requested a review July 5, 2024 05:30
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

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

@MSattrtand I created a new PR -- #241.

Also see my comments here -- #231 (comment)

Read my comments, close this PR, and continue work on the newly provided PR (make new PRs to merge to this branch)

@MSattrtand MSattrtand closed this Jul 8, 2024
@MSattrtand MSattrtand deleted the fix-issue-231 branch July 8, 2024 21:44
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