-
Notifications
You must be signed in to change notification settings - Fork 17
feat(#256): lint that find not merged bases #535
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
🚀 Performance Analysis
|
|
|
||
| ```xml | ||
| <o base="foo"> | ||
| <o base=".bar"/> |
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.
@Marat-Tim such example is not correct because .bar method should be taken from some object. So you need to add a nested object that can't be merged with .bar or just remove dot: bar
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.
@maxonfjvipon I forgot to delete this
| <object> | ||
| <o base=".foo"> | ||
| <o base="bar"> | ||
| A1-B2-C3-D4-E5 |
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.
@Marat-Tim there's something wrong with indentation here
| <xsl:output encoding="UTF-8" method="xml"/> | ||
| <xsl:template match="/"> | ||
| <defects> | ||
| <xsl:for-each select="//o[not(eo:abstract(.)) and parent::o[starts-with(@base, '.')] and not(starts-with(@base, '.')) and not(text())]"> |
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.
@Marat-Tim actually it's pretty good, but I think you missed the one more condition - and not(o). You can add this test:
<o base=".foo">
<o base="bar">
<o base="baz"/>
</o>
</o>I believe with current approach you'll get a defect, but there's no one
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.
@maxonfjvipon This is checked with not(text()) because if element has an inner object, text() returns its text content. I added this test
In this pull request, I’m adding a linter that checks whether an object can be merged with its parent. There were two potential approaches to implementing this: one involved identifying full chains of possible merges—for example, in the following code:
These objects could theoretically be merged into:
However, such a search is relatively complex in terms of either memory usage or computational resources
I opted for a simpler approach, where in the above example, the linter will generate a warning like:
Object on line 2 can be merged with its parentAfter performing the suggested merge and re-running the linter, another warning will appear, allowing the issue to be resolved over several iterations