-
Notifications
You must be signed in to change notification settings - Fork 2
Document path statements #71
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
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM
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.
A few comments, but let's hold off on landing this until Blocks are deployed to Prod.
docs/concepts/CLQL.md
Outdated
common.func: | ||
block(repeat = 3): | ||
common.if_stmt: | ||
blockcontinue |
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.
this needs a blockend
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.
I was thinking that blockend
is only necessary if you want to have multiple blockcontinue
statements, and continue them after the block.
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.
This is an implicit / explicit decision. Generally, I'm in the explicit camp, but we are tending towards implicit with CLQL.
What happens if we nest a second block within the first? Does that start a new top level loop or is it a nested loop? Explicit blockends would make it clear.
Having said that, at this stage, I would be persuaded by whichever is 1. the quickest path to implement and 2. most likely to be confusion / side effect free.
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.
I'm not sure if this is an implicit / explicit issue - I don't think my way is hiding anything from the user, but it does give a little bit of syntactic sugar.
I might have a different view of what blockend
means. Perhaps we could replace it with afterblock
. To me, blockend
means "match from here once you've finished with the block" - and if there is only one blockcontinue
it seems natural to put the "match from here" nodes underneath the blockcontinue
.
I think nested block statements would create a nested loop, I'm not sure how they could be interpreted otherwise. I give an example of nested blocks below (though I don't think we need to support them yet for implementation reasons), and it doesn't have blockends. This "follows the callgraph from someFunc
and via functions with multiply nested for loops":
common.func:
name == "someFunc"
block(repeat = any):
block(repeat = 2:):
common.for_stmt:
blockcontinue:
common.func_call(depth = any):
edge("calls"):
common.func:
blockcontinue
I don't know what the implementation/side effect differences are, but I think the two would be much the same.
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.
You wouldn't nest two blocks directly. What about:
block(repeat = any):
common.func:
common.var
block(repeat = 2):
common.if
blockcontinue
common.for
docs/concepts/CLQL.md
Outdated
common.func: | ||
block(repeat = 3): | ||
common.if_stmt: | ||
blockcontinue |
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.
You wouldn't nest two blocks directly. What about:
block(repeat = any):
common.func:
common.var
block(repeat = 2):
common.if
blockcontinue
common.for
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.
block -> path
don't allow repeat = any
- use depth ranges instead
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.
LGTM
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.
LGTM
|
||
Nested paths are not supported. | ||
|
||
Using `any_of` inside a path statement is not supported. |
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.
Does this mean that I must change this tenet to use a nested any_of? https://github.com/mullikine/codelingo/blob/psr/tenets/codelingo/psr-1/uppercase-class-constants/codelingo.yaml Will the path syntax used in the tenet be supported in the future? Also, is path expanded into a nested any_of before it is evaluated? Knowing this would help me to understand how I'm supposed to use it. For example, I'm questioning if I should put the depth = any inside the path or the path's children. This raises a couple of questions. Can I have multiple arguments to the path fact i.e depth and repeat? Can I have path at the root of the CLQL query?
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.
Yes it will be supported in the future, that's a key use case.
Can you give an example of where you would be choosing between putting a depth = any
inside the path or the path's children?
The path element only takes a repeat
argument, facts inside path can take depth
arguments. Element, by the way, is the generic term for fact, property, path, any_of, etc.
Yes you can have a path at the root.
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.
The case I had in mind when asking the question was the tenet I linked to above. Here I have "depth = any" specified for both children. But if a repeat was specified in the path then each nested child would have a "depth = any". I'm unsure what this would do to performance, but I'd imagine you might get a tetration thing going. Also, it kind of makes sense to place the "depth = any" within the path fact because then you're only specifying it once. Either that or place the "depth = any" on a zero-width parent to the path fact. Should this go into discuss?
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.
I still don't know what you're suggesting. I can't think of any argument repeat argument that you could pass to path that would replace the need for a depth = any
. Can you write up some CLQL?
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.
I made a topic on discuss to continue the conversation.
https://discuss.codelingo.io/t/clql-syntax-new-features/87
docs/concepts/CLQL.md
Outdated
@@ -53,7 +53,7 @@ To limit the above query to match classes with a particular name, add a "name" p | |||
|
|||
``` | |||
@review.comment | |||
common.method(depth = any): | |||
method(depth = any): |
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.
Is it encouraged to use common lexicon facts when go lexicon facts would do? Am I currently able to entwine common lexicon facts with go lexicon facts?
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.
Yeah you can, but don't worry about it for now. The common lexicon isn't properly implemented yet. common.func_decl
will match go.func_decl
or any other kind of func_decl
, but we haven't put any effort into making sure the different names for func_decl
in various languages map to the same common fact.
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.
LGTM
Document basic edge use case, as
block
is largely motivated by recursively following the call graph.