google / google/grr

Improve naming and linter checks in the new UI codebase

Open
#866 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
5.1k
Forks
796
PR merge metrics
No merged PRs in 30d

Description

**Environment**
* I followed [the tutorial for setting up a dev environment](https://grr-doc.readthedocs.io/en/latest/developing-grr/setting-up-dev-env.html).
* I'm developing in VScode, with the `ms-vscode.vscode-typescript-tslint-plugin` plugin installed, version `v1.2.2`
* The issue is with the new UI codebase: [`grr/server/grr_response_server/gui`](https://github.com/google/grr/tree/master/grr/server/grr_response_server/gui/ui). I haven't explored the rest of the code.

**Describe the issue**
There are a few tslint warnings that show up consistently across components. The ones that I've noticed are:
* Some [component selectors](https://github.com/google/grr/blob/master/grr/server/grr_response_server/gui/ui/components/flow_details/helpers/file_results_table.ts#L48) do not have a special prefix (e.g. to make them distinct from 3rd-party components). I get the following warning: `The selector should be prefixed by "app" (https://angular.io/guide/styleguide#style-02-07) (component-selector)tslint(1)`. If the component doesn't have dashes in the name like the [timestamp component](https://github.com/google/grr/blob/master/grr/server/grr_response_server/gui/ui/components/timestamp/timestamp.ts#L7) the exact error is: `The selector should be kebab-cased and include a dash (https://angular.io/guide/styleguide#style-05-02) (component-selector)tslint(1)`
* Some [component classes](https://github.com/google/grr/blob/master/grr/server/grr_response_server/gui/ui/components/client_details/client_details.ts#L13) don't have a `Component` suffix, which gives the following warning: `The name of the class ClientDetails should end with the suffix Component (https://angular.io/styleguide#style-02-03) (component-class-suffix)tslint(1)`

This is not a complete list, and there are probably more tslint warnings.

Shouldn't those warnings be checked in the CI?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.