-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Add new output format markdown #258
Conversation
Any reason why text path is the full path and json is relativ?: Text: JSON: |
Thank you very much for this PR! We'll discuss it in the team and get back to you.
The full path in the text output allows you to jump directly to the finding in some IDEs. In VSCode for example, such paths will be rendered as a link that you can "ctrl+click" to open the file and even scroll to the correct position in the file. Since the JSON output is not intended for direct consumption by developers but rather for automated reportings or statistics, we decided to keep the paths relative to save on file size and to not include potentially sensitive path segments from a developers local system. ESLint always uses absolute paths in its output: https://eslint.org/docs/latest/use/formatters/#stylish |
I reviewed the code already last week and have no objections. Regarding the paths I think we should format them relative to the project root. In addition, we played around with the PR and found the formatting was not ideal in all cases. Especially when there are many findings in a single file, and some are warnings while others are errors (or fatal errors) we should think whether a sorting based on the severity (and then by position) makes sense. I'm also unsure of the use of emojis for illustrating the severity. I believe the two circles 🟡 🔴 might be hard to distinguish for individuals with visual impairment. We have an intern this summer (@konnraad) and tasked him with creating a table-version based on your PR, see below. He also added the mentioned sorting based on severity. Please let us know what you think: Table output sampleUI5 Linter ReportFindings per Fileui5.yaml
webapp/Component.js
webapp/controller/App.controller.js
webapp/manifest.json
webapp/view/App.view.xml
webapp/view/Demo.view.xml
Summary
The drawback when using a table is that it's harder to read in plain-text or in the console output compared to your initial proposal. But we are not sure whether this is a valid use case for you anyways. Should we focus on having a nice format in the rendered markdown or is the plain text format important as well? (CC: @marcelschork) Looking forward to your feedback. Our intern is also standing by to make the necessary changes depending on what we agree on 😉 |
Hi Merlin, I actually don't have (yet) a use case for markdown. I actually tried to implement the |
@RandomByte I very much like the table format because the readability is much better now (which is essential for PR decoration). I think if somebody choose the format option |
Grouping by rules has been removed with SAP#6 but it was missed to remove the corresponding data structures.
We moved the summary to the top. Sorting is now by severity (order: Fatal Errors, Errors, Warnings), then line, then column. The output looks like this now: Updated Table output sampleUI5 linter ReportSummary
Findingsui5.yaml
webapp/Component.js
webapp/controller/App.controller.js
webapp/manifest.json
webapp/view/App.view.xml
webapp/view/Demo.view.xml
|
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.
LGTM
@marianfoo we appreciate your opinion on the current state of this PR. If you are satisfied with the current state we will follow-up with the merge of the PR and produce a new release. |
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.
LGTM
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.
LGTM. Thanks!
Released with https://github.com/SAP/ui5-linter/releases/tag/v0.3.4. Thanks a lot @marianfoo for your contribution! |
fix #178
Added new
markdown
option for output as suggested in the issue.Added tests for output.
Here is a sample output with
ui5lint --format markdown --details
:Markdown output sample
UI5 Linter Report
/Users/marianzeis/DEV//webapp/control/DynamicCompartmentBox.ts
write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API.write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API.write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API./Users/marianzeis/DEV//webapp/control/DynamicCompartmentBoxItem.ts
write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API.write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API.write
. Typically,write
is used to create a longer sequence of HTML markup (e.g. an element with attributes and children) in a single call. Such a markup sequence has to be split into the individual calls of the Semantic Rendering API./Users/marianzeis/DEV//webapp/localService/mockserver.js
/Users/marianzeis/DEV//webapp/util/Util.ts
/Users/marianzeis/DEV//webapp/view/Source.view.xml
enableExport
property.rowMode
aggregation instead.rootLevel
binding parameter. May not work with all bindings.numberOfExpandedLevels
binding parameter. May not work with all bindings./Users/marianzeis/DEV//webapp/view/Target.view.xml
rowMode
aggregation instead./Users/marianzeis/DEV//webapp/view/TargetBadHU.view.xml
rowMode
aggregation instead.Summary
Text output sample
JSON output sample
JIRA: CPOUI5FOUNDATION-890