Support (line-based) disabling of xml and js rules

[richardatsap]

As there are inevitably false positives (at least in my projects), it would be nice if there where the possibility to disable rules at least line based.

For a first version it would be enough to switch off xml or js linting entirely for the line.

[iljapostnovs]

Hi, how do you logically imagine line based linter disabling? What exactly should be disabled e.g. in this case:

Or in this:

[richardatsap]

Hi,

good question ;-). For a first version I think it would be fine if everything in the enclosing scope of the following element would be disabled (aside from invalid xml which never should be disabled).

For your examples I would expect:

1. Disabling all linting within the scope.
2. Good catch! I didn't think of this formatting variant (bad practice anyways). Disabling everything within the first scope. In other words <Button ..../> should still be linted. People should format correctly.

So, not exactly line-based for xml linting (owed to the fact that xml does not know line comments).

A more fine granular version might be: Either disabling certain checks by their id (the check hover than should state the id) to disable only this check in the scope (like eslint does). Or denoting an element or attribute would disable the checks only for that attribute or element in the scope. I think the first variant would be more in line with what people are used to with other linters.

Kind Regards Richard

[iljapostnovs]

Hi,

good question ;-). For a first version I think it would be fine if everything in the enclosing scope of the following element would be disabled (aside from invalid xml which never should be disabled).

For your examples I would expect:

1. Disabling all linting within the scope.
2. Good catch! I didn't think of this formatting variant (bad practice anyways). Disabling everything within the first scope. In other words <Button ..../> should still be linted. People should format correctly.

So, not exactly line-based for xml linting (owed to the fact that xml does not know line comments).

A more fine granular version might be: Either disabling certain checks by their id (the check hover than should state the id) to disable only this check in the scope (like eslint does). Or denoting an element or attribute would disable the checks only for that attribute or element in the scope. I think the first variant would be more in line with what people are used to with other linters.

Kind Regards Richard

Hi, I had the same idea about "id"-based disabling, seems like more bulletproof solution as for me. Moreover, there is a possibility to refer to specific attributes which should be ignored.

Didn't really get the idea about "scopes" here and comparing to eslint. ESlint works differently, it disables errors for next line, however this logic will not fit for xml (because of tag attributes). And there is no way to write the plugin hoping that "people should format code well" :) The only possible thing which comes into my mind would be that @ui5ignore could disable all errors for next tag after it.

[richardatsap]

Hi,

sorry for not being precise here. With "scope" i meant the tag following the comment and everything it encloses (attributes and tags).

Kind Regards Richard

[iljapostnovs]

Hi,

sorry for not being precise here. With "scope" i meant the tag following the comment and everything it encloses (attributes and tags).

Kind Regards Richard

Hi, No worries. Do you mean that @ui5ignore should disable errors for tag children as well? E.g. in this case List and DisplayListItem errors should be disabled?

If yes, I think that it would be better option to disable errors for List only, and if someone wants to disable errors for DisplayListItem, it would be possible to add another @ui5ignore there as well

[richardatsap]

Hi,

your approach is better as it allows more fine granular control, especially with deeply nested controls.

[richardatsap]

Hi,

sorry to bother again. With this request I asked for line based disabling of js-linting as well (for example setting a comment "// @ui5ignore" at the end of the line. I have a bunch of classes in my project whichs attributes are not recognized correctly by the linter and it'd be nice to be able to exclude those false positives as well.

If you prefer I can open a separate issue for that.

[iljapostnovs]

Hi, There are two options how to disable linters in js: 1) Use @ui5ignore tag in jsdocs to disable all linters for the method 2) Add exceptions to VSCode preferences.

This should be enough, are there any situations where those two options doesn't help?

[richardatsap]

Hi,

1. I would prefer to not loose all checks in a method (I have rather longish ones). The linting is very helpful it already hinted me to subtle problems.
2. As per attached screenshot, the JSDoc comment does not work in all cases

[richardatsap]

Hi,

the xml ignore command switches off the tag linter, but not the file linter as per screenshot below.

Otherwise, I am impressed with the fast deployment of new functionality!

[iljapostnovs]

Hi,

1. I would prefer to not loose all checks in a method (I have rather longish ones). The linting is very helpful it already hinted me to subtle problems.
2. As per attached screenshot, the JSDoc comment does not work in all cases

It's a bit wrong usage of @ui5ignore. Tag disables linting of the method itself, meaning that if you have errors with calling "_onRouteMatched" method, @ui5ignore disables it, but it does not disable errors in the method body. Regarding NavigationEventType I can't say anything because there is nothing in the screenshot what could hint why you get the warning (and what exactly warning do you get). But for such cases there are preferences in vscode which can be set on workspace level.

Hi,

the xml ignore command switches off the tag linter, but not the file linter as per screenshot below.

Otherwise, I am impressed with the fast deployment of new functionality!

Yeah, seems fair enough... In this case I don't see how can I distinguish difference between static method call and class name. UI5Ignore tag can't disable it because it is working by principle "Please find all strings in the file which starts with component name". Meaning that there is no tag/attribute parsing used for the linter. In this case I would vote for correct MVC usage. It should be always the controller which handles events from view, and from the controller you can call whatever is necessary.

[richardatsap]

It's a bit wrong usage of @ui5ignore. Tag disables linting of the method itself, meaning that if you have errors with calling "_onRouteMatched" method, @ui5ignore disables it, but it does not disable errors in the method body. Regarding NavigationEventType I can't say anything because there is nothing in the screenshot what could hint why you get the warning (and what exactly warning do you get). But for such cases there are preferences in vscode which can be set on workspace level.

NavigationEventType is not derived from sap.ui.base.Object which probably is the reason for this particular problem. However it'd be nice to have more fine granular control over linting.

[richardatsap]

Yeah, seems fair enough... In this case I don't see how can I distinguish difference between static method call and class name. UI5Ignore tag can't disable it because it is working by principle "Please find all strings in the file which starts with component name". Meaning that there is no tag/attribute parsing used for the linter. In this case I would vote for correct MVC usage. It should be always the controller which handles events from view, and from the controller you can call whatever is necessary.

This one is strange because it works for other utilities I use, see screenshot. Both are defined in the very same way I was not able to find what may cause the issue. I'll open a separate bug report when I found out how to reproduce them problem.

[iljapostnovs]

It's a bit wrong usage of @ui5ignore. Tag disables linting of the method itself, meaning that if you have errors with calling "_onRouteMatched" method, @ui5ignore disables it, but it does not disable errors in the method body. Regarding NavigationEventType I can't say anything because there is nothing in the screenshot what could hint why you get the warning (and what exactly warning do you get). But for such cases there are preferences in vscode which can be set on workspace level.

NavigationEventType is not derived from sap.ui.base.Object which probably is the reason for this particular problem. However it'd be nice to have more fine granular control over linting.

I'm still missing the idea of what granularity is not there? Are there any reasons why you can't add the exception to vscode settings?

Yeah, seems fair enough... In this case I don't see how can I distinguish difference between static method call and class name. UI5Ignore tag can't disable it because it is working by principle "Please find all strings in the file which starts with component name". Meaning that there is no tag/attribute parsing used for the linter. In this case I would vote for correct MVC usage. It should be always the controller which handles events from view, and from the controller you can call whatever is necessary.

This one is strange because it works for other utilities I use, see screenshot. Both are defined in the very same way I was not able to find what may cause the issue. I'll open a separate bug report when I found out how to reproduce them problem.

I just debugged the linter and turns out I have a check if the last member is a method in the specific class, so yes, actually previous example with linter error should have worked fine for you. If you get any idea how to reproduce or share the example, I could debug it.

[richardatsap]

I'm still missing the idea of what granularity is not there? Are there any reasons why you can't add the exception to vscode settings?

I'd like to disable messages related to NavigationEventType but I'd like to get warnings if I use a non-existent method from other classes. As far as I understand if I uncheck "Use Wrong Field Method Linter" I get none of the warnings even the ones I'd like to see.

[iljapostnovs]

I'm still missing the idea of what granularity is not there? Are there any reasons why you can't add the exception to vscode settings?

I'd like to disable messages related to NavigationEventType but I'd like to get warnings if I use a non-existent method from other classes. As far as I understand if I uncheck "Use Wrong Field Method Linter" I get none of the warnings even the ones I'd like to see.

True, I wasn't transparent regarding the settings I am talking about. Here you can apply which member of which class should not be considered as error. E.g. {"memberName": "setParent", "className": "sap.ui.core.Control", "applyToChildren": true}, means that setParent of sap.ui.core.Control class should not throw any errors, as well as all children-classes of it.

[richardatsap]

This one is strange because it works for other utilities I use, see screenshot. Both are defined in the very same way I was not able to find what may cause the issue. I'll open a separate bug report when I found out how to reproduce them problem.

I just debugged the linter and turns out I have a check if the last member is a method in the specific class, so yes, actually previous example with linter error should have worked fine for you. If you get any idea how to reproduce or share the example, I could debug it.

What I found so far: I have two helper classes "MasterEditorUtility" and "DetailEditorUtility". For the former none of the member or static methods are recognized neither in my .js files nor in my xml files. For the latter both works fine (member methods or static methods in .xml and .js files).

I also tried formatting "MasterEditorUtility", to no avail, I even searched for tabs followed by a space.. Is there anywhere in VSCode a view where I could see whether the parser throws errors (like the error view in Eclipse)?

[iljapostnovs]

It is possible to enable debugger in VSCode:

However, it will not give you much information, because the plugin is minified. It would be rather easier if you could provide an example.

[richardatsap]

Hi,

thanks. It seems the parser you use can parse "DetailEditorUtility" but not "MasterEditorUtility". The UI5 Explorer shows the correct tree for the former but no method or property for the latter. Unfortunately I cannot provoke the parser to emit a message on the console (I tried "refresh" on the UI5 Explorer).

I suspect that formatting problems as described on your Readme.md is the cause. However an automatic format run did not solve the problem (I restarted VSCode).

[iljapostnovs]

Hi,

I don't think that it's formatting problems, VSCode formatter usually fixes them. The only known issue to me is that plugin might fail if you use JSON notation for field declaration, e.g.: "metadata": {"properties":{}} instead of metadata: {properties:{}}. Impossible to guess the reason without an example.

[richardatsap]

Hi,

found the reason.

Indentation like this:

    const MasterEditorUtility = UI5Object.extend(
        "sap.gtnc.controller.MasterEditorUtility", {
        _oController: null,
        _oFilterBar: null,
        _oBasicSearchField: null,
        _oTable: null,
        _oPage: null,
        _oEditorBaseUtility: null,
        _oVariantPopover: null,
        _bDefaultVariantApplied: false,
        _bColumnArrangementApplied: false,
        _mEntityPropertyToCellMap: null,
        _mEntityPropertyToColumnMap: null,
        _oTableSortDialog: null,
        _oSearchValueModel: null,
        _bVariantMarkedDirty: false,
        _bStandardVariantsChecked: false,

does not work, whereas indentation like this:

    const MasterEditorUtility = UI5Object.extend(
        "sap.gtnc.controller.MasterEditorUtility", {
            _oController: null,
            _oFilterBar: null,
            _oBasicSearchField: null,
            _oTable: null,
            _oPage: null,
            _oEditorBaseUtility: null,
            _oVariantPopover: null,
            _bDefaultVariantApplied: false,
            _bColumnArrangementApplied: false,
            _mEntityPropertyToCellMap: null,
            _mEntityPropertyToColumnMap: null,
            _oTableSortDialog: null,
            _oSearchValueModel: null,
            _bVariantMarkedDirty: false,
            _bStandardVariantsChecked: false,

does work. Very weird. I could confirm this behavior with other files.

[iljapostnovs]

Hi, both options works for me If i copy-paste it.

[iljapostnovs]

Ok nevermind, VSCode formatted it for me, that's why it was working. Well, that's acorn issue. I guess I will add to readme about what exactly am I using as formatter and what are the settings.

[iljapostnovs]

Did you manage to use vscode preferences to add exceptions for your case? If yes, I think we can close this issue.

[richardatsap]

Hi,

sorry for the delay, I had been on a short trip.

The use-case in the screenshot below would require a line based ignore comment. Neither @sapui5ignore nor specifying the method in the settings.json got rid of the warnings. Your plugin detects correctly the usages as event handler. The handler is called with the controller as "this". Note, that the false positives end with the scope of the lambda although it's "this" contains the same object as the outer context. I am not expecting detection of all corner cases (see below), but commenting out the false positives would be helpful.

Other corner cases found so far:

1. The linter cannot detect the concrete type of the returned subtype of Element when retrieved with View.byId() and stored as member. This leads to warnings when using methods of the concrete type.
2. The concrete type of members is not detected in some cases, which leads to the same problem.

Once again, Javascript is dynamic enough to not expect a linter (or parser) to infer types correctly. Therefore a line-based disabling comment would be nice. It even would be enough to have the comment on the same line if that saves you headaches in implementation.

Like: this.renameEntity(); //@ui5ignore

[iljapostnovs]

Hi,

I don't really get the exact problem here. On the screenshot above I can't see what message do you get and why. Could you provide some code snippets for the problems you are referring to? Then I will be able to comment what exactly you are getting and why.

Incorrect warnings usually shows up because of the incorrect OOP usage and taking advantage of JS dynamics when it is not necessary. First case sounds more like a problem which could be fixed by adding @type jsdoc to variables. Second one is too abstract and doesn't provide any concrete information about the problem.

I don't want to add inline disabling of the linter for several reasons: 1) Acorn is not parsing comments 2) Usually (not always) the reason of the warning is bad quality architecture which is created not according to OOP (or MVC specifically) principles. So perhaps for somebody seeing the warnings would give a signal that something is not developed correctly and next time people would develop in more "expected" way. 3) There are settings in vscode, @type tag and @ui5ignore tag supported, which should solve basically all problems if app architecture is not a mess.

[richardatsap]

Hi,

as for the screenshot: DetailEditorUtility.onSubmitChanges serves as a "static" event handler (DetailEditorUtility is not a controller class). It is used as event handler for a bunch of views as your plugin correctly recognizes (13 references called out right in the first line). UI5 injects the controller of the view where the event handler is called as "this" for the function call. Therefore, within nthe event handler I can call this.getOwnerComponent(). However the linter marks the line with

"getOwnerComponent" does not exist in "sap.gtnc.controller.DetailEditorUtility"

as the linter seems to assume "this" being an instance of "sap.gtnc.controller.DetailEditorUtility".

[iljapostnovs]

This is exactly what I am talking about - unnecessary JS dynamics usage. "this" context is dynamically changed to another class instance. Please use more object oriented way to deal with those situations. Swapping the context is not only not the best practice to develop, but it also ruins readability. You will never expect that the context is swapped if you look at the code for the first time. That's totally fine to have utilities, however swapping the context is never the best way to go. According to MVC pattern it's always the controller which handles view events (not static classes with swapped context), that's the first violation. Secondly, I assume that there are a lot of utilities because model is misused as well. According to MVC pattern, model should be always handling all data-related logic, like submitting, deleting, manipulating with data etc. When you push all data related logic to model, you will find out that controller is never large and it will never need additional utilities to handle view events. Please consider developing according to object oriented principles, SOLID principles and MVC pattern, it will make the code more readable and maintainable. In case of the plugin, it will help to avoid false warnings as well.

[richardatsap]

Hi,

the helper classes are used to avoid repetitive code. The only alternative would be using deep inheritance hierarchies which come with their own caveats. As well, some use-cases cannot be solved using inheritance therefore it was decided to use helper classes with composition. We cannot change the code base now (it's rather big). Furthermore, UI5 does support static handlers so it'd be nice if one could use them with your tool. I found the linting very valuable I would regret being forced to switch it off.

As for the concrete problem, one could annotate the method with @this (https://jsdoc.app/tags-this.html) to make it transparent to which instance this is referred: but unfortunately the linter doesn't seem to honor @this unlike @type annotations. I think this would be a good compromise (forcing the code base to use @this to satisfy the linter).

[iljapostnovs]

Respecting @this tag is a big effort, the parser is not built in the way that for some reason anybody could want to swap the context. I understand that it's not easy to rebuild code base, but again, hopefully next time there will be more time spent on thinking about architecture :) If there are static methods called from the view, it's not the composition. If you would use composition, you would have zero problems. This is composition:

This is what you have (at least from what I understood):

As I told, not the greatest architecture according to OOP and MVC pattern. And it's a huge effort from my side to support such cases. Moreover, I would never want to see this in production :) Sorry, but I will not introduce support for such cases. It's against OOP and MVC pattern.

This is how it should look like according to MVC:

If model gets too complicated, utilities can be introduced as well:

[richardatsap]

Hi,

thanks for your elaborate answer. I respect your stance although I do not share everything of it ;-).

For the concrete problem I found the following workaround

Kind Regards Richard

[iljapostnovs]

Hi,

thanks for your elaborate answer. I respect your stance although I do not share everything of it ;-).

For the concrete problem I found the following workaround

Kind Regards Richard

That's an interesting workaround, didn't come into my mind. Good luck in coding!