-
Notifications
You must be signed in to change notification settings - Fork 160
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
kernel: unify 'and' for filters in interpreter and executor #2755
Conversation
a8008b3
to
2b48e81
Compare
Error, <expr> must be 'true' or 'false' (not a function) | ||
gap> function() return Center and IsAssociative; end(); | ||
Warning, operation 'Centre' used in AND-filter is not a filter in stream:1 | ||
<Filter "(Centre and IsAssociative)"> |
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.
Here the warning can be seen in effect
Codecov Report
@@ Coverage Diff @@
## master #2755 +/- ##
==========================================
+ Coverage 76.03% 76.03% +<.01%
==========================================
Files 480 480
Lines 240919 240946 +27
==========================================
+ Hits 183185 183214 +29
+ Misses 57734 57732 -2
|
I wonder if we should not merge this until after 4.10 branches, to give packages a full release cycle to fix bugs, rather than have warnings in 4.10.1? Or is it important those bugs get fixed ASAP? |
No, it's not important at all. I just suggested it as a possibility in PR #2732, and @ChrisJefferson wrote:
;-). But I am also fine with skipping this PR completely, and just merging PR #2756 (squashed down) once all affected packages made a fixed release. |
Also start using IS_FILTER. Note that previously, this input was silently accepted: gap> Center and IsAssociative; <Filter "<<and-filter>>"> This was effectively treated equivalently to `HasCenter and IsAssociative;`. We now could reject such code. But unfortunately a few packages accidentally relied on this bug. We thus only print a warning for now, and will turn this into a proper error only once all affect packages released a fixed version.
2b48e81
to
54c34b7
Compare
Hmm, just realized that there should be a third place that needs to be adjusted: the compiler. Gotta do that, including a test |
Closing this in favor of PR #2756 |
Also start using IS_FILTER. Note that previously, this input was silently accepted:
This was effectively treated equivalently to
HasCenter and IsAssociative;
.We now could reject such code. But unfortunately a few packages accidentally
relied on this bug. We thus only print a warning for now, and will turn this
into a proper error only once all affect packages released a fixed version.
This continues the work begun in PR #2732