doc/flowint: fix terminology for unset variable - v1 #12159
Conversation
s/isnotset/notset, for flowint Flowints don't follow the same pattern as other similar keywords, but the docs indicated otherwise.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12159 +/- ##
==========================================
+ Coverage 49.81% 49.84% +0.03%
==========================================
Files 909 909
Lines 257904 257904
==========================================
+ Hits 128467 128547 +80
+ Misses 129437 129357 -80
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -143,7 +143,12 @@ Define a var (not required), or check that one is set or not set. | |||
| :: | |||
|
|
|||
| flowint: name, < +,-,=,>,<,>=,<=,==, != >, value; | |||
| flowint: name, (isset|isnotset); | |||
There was a problem hiding this comment.
My instinct is that we should keep such things consistent among the rule keywords. Of course, that comes at an expense of changing and breaking the existing rulesets that use this.
There was a problem hiding this comment.
Should we rather change the code to support both historical notset but also generic isnotset ?
There was a problem hiding this comment.
Fair point. Should I create a ticket, then?
There was a problem hiding this comment.
There was a problem hiding this comment.
Why change the code? Why not just keep this to updating the docs to reflect reality?
There was a problem hiding this comment.
As mentioned above and in the ticket, to keep consistency among rule keywords syntax. Should we not aim for that?
There was a problem hiding this comment.
I'm more referring to why close this one which corrects the documentation issue. The code change is a somewhat separate issue which is still good to have for consistency.
There was a problem hiding this comment.
Code change + doc will be a logical unit of some sort...
But let's not overthink it, I am ok either way
There was a problem hiding this comment.
I guess... We can merge this, and then once the code change happens, another doc update will be due.
|
|
||
| .. note:: | ||
|
|
||
| It's important to observe that while similar keywords use ``isnotset``, |
There was a problem hiding this comment.
The proper fix here is to add isnotset support, while keeping the notset for compatibility reasons
There was a problem hiding this comment.
Yes, this seems trivial enough. But not getting to it, should not prevent correct documentation :)
|
Adding |
s/isnotset/notset, for flowint
Flowints don't follow the same pattern as other similar keywords, but the docs indicated otherwise.
--
The examples are correct but if someone only read the explanation (as I did, they'd end up with a broken rule).