fallthrough="true" is redundant with fallthroughContext="xxx" and there is no reason to use fallthrough="false"
Details
Details
- Reviewers
dhaumann cullmann - Group Reviewers
Framework: Syntax Highlighting - Commits
- R216:c885eaa69d73: implicit fallthough if there is fallthoughContext
Diff Detail
Diff Detail
- Repository
- R216 Syntax Highlighting
- Branch
- auto_fallthrough (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14891 Build 14909: arc lint + arc unit
Comment Actions
I appreciate the removal of the fallthrough=bool attribute.
Given it is sematic-free if no fall through context is there I guess this is a "compatible" change.
But please update the XSD (there are even some comments about fallthrough being dubious)
Comment Actions
I like this change a lot, but there is a huge (theoretical) backwards compatibility issue with this.
Before, the code ignored fallthroughContext if m_fallthrough was explicitly set to false. So you could have: fallghrough="false" fallthroughContext="abc", and fallthroughContext was ignored.
Now this changed, and m_fallthough solely depends on whether fallthroughContext is a valid context or not other than #stay.
My point is: For changes like this, we usually have to
- increase the version number in the *.xml files
- increase the kateversion from 5.0 to 5.62?
- Since we have IncludeRules, wouldn't we even need to increase the kateversion number of _all_ xml files to be on the safe side?
I think these issues first need to be clarified before we can commit this.
Comment Actions
Hmm, isn't any file with some fallthrough=false but a fallthrough context just buggy? Did any of our files have that?
Comment Actions
I think it's an improvement, but the ideal would be to maintain support for fallthrough="bool". That is, if the fallthroughContext attribute is present, assume fallthrough="true", unless "fallthrough=" is explicitly written.
What I don't agree with is modifying all *.xml files, since this implies using kateversion="5.62", which will generate problems for those users who install the *.xml files manually in previous versions of KDE Frameworks (and not all GNU/Linux distributions have the latest version). This, considering that this is just a "cosmetic" change in the code of the *.xml files.
So, I propose two alternatives:
- Maintain support for fallthrough="bool", as an optional attribute (i.e., what I wrote at the beginning), as a transition and in a future version, such as KF6, completely eliminate the "fallthrough" attribute, since as jpoelen says this is redundant.
- Now remove support for the "fallthrough" attribute (what jpoelen proposes), but don't modify the *.xml files to maintain the compatibility of these files in older versions of KF5. No *.xml files uses fallthrough="false" or "fallthroughContext" alone, so this doesn't cause regression. In that case, it should be announced in the documentation that this attribute is obsolete.
Comment Actions
I will hand over the original xml files, that seems preferable to me too.
I did not see fallthrough="false" nor fallthrough="0". But after checking, brightscript.xml uses` fallthroughContext` twice without fallthrough. in my opinion, it's an oblivion with no unfortunate consequences.
Comment Actions
- don't modify the *.xml files to maintain the compatibility in older versions of KF5
- langauge.xsd: accept only 1 or true for fallthrough attribute
Comment Actions
I think this is much better. For KF6, we can remove it in the xml files as well or so...
Besides that: why does the diff show so many changes even though it's just about fall through? It makes the review harder (especially on a smartphone).