ENT-13841: Fixed linting not working with namespaced bundle/body#41
ENT-13841: Fixed linting not working with namespaced bundle/body#41olehermanse merged 1 commit intocfengine:mainfrom
Conversation
SimonThalvorsen
force-pushed
the
ENT-13841
branch
from
8aae5b1 to
2f92553
Compare
SimonThalvorsen
force-pushed
the
ENT-13841
branch
2 times, most recently
from
a6c3ce3 to
ada6619
Compare
Ticket: ENT-13841 Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>
SimonThalvorsen
force-pushed
the
ENT-13841
branch
from
ada6619 to
3525054
Compare
| if guard is None: # Should never happen | ||
| print("ERROR: Bundle section without a promise guard") | ||
| return |
There was a problem hiding this comment.
| if guard is None: # Should never happen | |
| print("ERROR: Bundle section without a promise guard") | |
| return | |
| assert guard |
| return _State(block_type=self.block_type) | ||
| # A bundle_section is always: promise_guard, [promises], [class_guarded_promises...] | ||
| # The promise_guard is guaranteed to exist by the grammar | ||
| guard = next((c for c in node.children if c.type == "promise_guard"), None) |
There was a problem hiding this comment.
@SimonThalvorsen Why is this this necessary, isn't promise_guard always node.children[0] ?
| guard = next((c for c in node.children if c.type == "promise_guard"), None) | |
| assert node.children | |
| guard = node.children[0] |
| @staticmethod | ||
| def qualify(name: str, namespace: str) -> str: | ||
| """If name is already qualified (contains ':'), return as-is. Otherwise prepend namespace.""" | ||
| return name if ":" in name else f"{namespace}:{name}" |
There was a problem hiding this comment.
No big deal, but I would probably not make this a staticmethod. Can just be a top level function. Maybe prefixed with underscore to make it "internal".
| self.attribute_name = _text(child) | ||
| if self.attribute_name == "namespace": | ||
| self.namespace = _text(child.next_named_sibling).strip("\"'") |
There was a problem hiding this comment.
I see 2 problems with this:
- We don't want to do this whenever attribute name is
namespace. We want to do it only insidebody file control. So there should be another condition in the if to see if we are inside the right place. - Why use named sibling when we've set up the perfect state tracker to solve this kind of check? IMO we should set this when we get to the rval string (and check what block and attribute we are inside with an appropriate if check).
| state = _State() | ||
| ret = _stateful_walk(filename, lines, node, user_definition, strict, state=state) | ||
| state = _State() # Clear state | ||
| return ret |
There was a problem hiding this comment.
Clear state for what? Isn't this exactly the same as:
| state = _State() | |
| ret = _stateful_walk(filename, lines, node, user_definition, strict, state=state) | |
| state = _State() # Clear state | |
| return ret | |
| return _stateful_walk(filename, lines, node, user_definition, strict, state=_State()) |
| None, | ||
| ) | ||
| if name_node is not None: | ||
| promise_blocks.add(_text(name_node)) |
There was a problem hiding this comment.
I don't think this will work. I think you need to change your approach to reuse the same code across both passes. At a high level we need to:
- Parse all the files and convert them into syntax trees. This step will error on syntax errors.
- Loop over all the syntax trees and "discover" definitions. This step should not error (actually might in the future, if we want to add detection for duplicate definitions, for example).
- Loop over all the syntax trees again and actually perform our checks. This is where we will error for all the linting rules.
Step 2 and 3 should both use the _stateful_walk function IMO. It can have an argument for mode - for example "discovery" | "linting".
I understand the desire to do step 2 in a simple way as you have it here (just find and collect all the bundles and bodies). However, I don't think it's that simple, you need to have the same / similar context available as in step 3. You need to know which namespace you are in. In the future, if we do something similar for classes and variables, you need to know which bundle you are in, etc.
Another thing we have not handled at all yet is macros - the state could keep track of which macros we are "inside" (@if minimum_version for example).
| None, | ||
| ) | ||
| if ns_attr is not None: | ||
| ns = _text(ns_attr.next_named_sibling).strip("\"'") |
There was a problem hiding this comment.
This has the same problem as your other logic for detecting namespace - we only want to change namespace using a body file control block, not any attribute or any body.
2 participants
Ticket: ENT-13841