-
Notifications
You must be signed in to change notification settings - Fork 32
(Closes #483) inline directives should be comments (usually) #488
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
Conversation
|
It seems somewhat tricky to solve, since I'm not sure how to determine if the comment is inline or not, I need to dive deeper into how this all works to understand it. |
|
@LonelyCat124 Another thing to look at is that if we want to create unsupported directives in psyclone currently we should use a codeblock: Currently this files in the backend because |
I don't quite understand how/why this differs for the directives created via the frontend? I suspect they're created as |
|
Python 13.9 now throws an error for tests with this: This apparently didn't work before, so I'll remove the relevant mark. |
|
@sergisiso The base issue here is solved, I've not added the tofortran function as I'm a bit confused right now as to why this doesn't work for the manually created vs PSyclone created directives. |
|
@sergisiso - taken from teams: Checking this Sergi's declaration of the CodeBlock maybe causes the issue? In the backend the get_ast_nodes call returns this: The Do you want me to add the ability to make a Directive from a codesnippet directly? Edit: Actually I'm not sure that would be so easy, I think it really might need the Edit2: Not really - the creation of a |
sergisiso
left a comment
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.
@LonelyCat124 The functionality to prevent marking inlined comments as directives looks good but I am not sure about the removed fixture, see inline.
I am happy to move the Directive(string) to a separate issue.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #488 +/- ##
=======================================
Coverage 92.19% 92.19%
=======================================
Files 86 86
Lines 13802 13806 +4
=======================================
+ Hits 12725 12729 +4
Misses 1077 1077 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sergisiso I changed to your suggestion, and from reading the docs it should be ok so back to you. |
sergisiso
left a comment
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.
@LonelyCat124 Everything looks good now. Ready to merge.
No description provided.