Skip to content

Conversation

@dididy
Copy link
Contributor

@dididy dididy commented Aug 29, 2025

What is this PR for?

This PR for zeppeling-web-angular

In npm run lint, projects folder is linted using ng, but prettier only checks the src folder. In lint-staged, neither lint nor prettier checks the projects folder. This seems to need fixing.

Because ng lint --fix is not supported, I added a lint:fix script to run tslint with the --fix option.

Since src and projects folders need to reference different tslint configs, I added the corresponding option to lint-staged.

What type of PR is it?

Bug Fix
Improvement

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? N
  • Is there breaking changes for older versions? N
  • Does this needs documentation? N

Copy link
Contributor

@tbonelee tbonelee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about broadening the checks to all files except for dist, node_modules, and target (by using ignore files), instead of limiting them only to src and projects? I thought including configuration files as well would help keep things tidy and improve collaboration.

For example:

- "{src,projects}/**/*.{ts,js,json}": [
+ "**/*.{ts,js,json}": [

@dididy
Copy link
Contributor Author

dididy commented Aug 29, 2025

I’ve made the changes based on your feedback. Thank you, as always, for your valuable suggestions and review. Please let me know if anything else is needed.

@tbonelee
Copy link
Contributor

Could you please rebase this branch onto master?

Also, while reviewing, I noticed two items that might be worth double-checking:

  1. src/**/*.ts excluded from linting
    From what I can tell, src/tslint.json extends the root tslint.json, and the root's linterOptions.exclude seems to include src/**/*.ts. With the currnet configuration, I wasn't seeing lint errors for files under that path.
  2. --project value
    The TSLint CLI does indicate --project should point to a tsconfig rather than tslint.json. In my quick local check, passing -p src/tslint.json didn't enable type-aware rules (so some issues didnt' surface), whereas -p src/tsconifg.app.json did.
    Minimal repro:
  • Remove the linterOptions.exclude entry so that src/**/*.ts are not excluded.
  • Add:
// src/app/arr.ts
export const strs: string[] = [];
// src/app/for-in.ts
import { strs } from '@zeppelin/arr'; // use path alias so type info is needed
// Should trigger no-for-in-array
for (const s in strs) console.log(s);
  • Results:
    • npx tslint src/app/for-in.ts -> warnings: "requires type information".
    • npx tslint -p src/tslint.json src/app/for-in.ts -> no warnings, no errors (Maybe it was recognized as empty tsconfig file.)
    • npx tslint -p src/tsconfig.app.json src/app/for-in.ts -> ERROR: for-in loops over arrays are forbidden...

Tentative proposal (very open to correction):

Maybe we could use ng lint --fix to correct project-overall lint errors. Using this, we could ignore improper directories (node_modules, etc.) and lint for overall project codes automatically. If we add some other new directories(e.g., custom-rules) that we want to lint, then they should be added into angular.json as a new project. It could not check root-level .ts files, but I think they do not exist for now and it could be ignored for linting because those would be just some configurations.

Apologies for the long comments. My checks were somewhat inductive, so I might have drawn the wrong conclusions. I mainly wanted to see whether the previous code intentionally relied on passing tslint.json to --project (and whether that has any effect). If I've missed something, please correct me so we can wrap this up properly — and I'm open to any other suggestions as well.

@dididy
Copy link
Contributor Author

dididy commented Sep 2, 2025

Due to editor autocomplete, an incorrect parameter was passed to the -p option, and some mistakes occurred while making corrections based on that. Thanks again for the thorough review. I have removed linterOptions.exclude from tslint.json, and also removed the -p option when running tslint. (After removing the -p option, I confirmed that lint works correctly based on tslint.json)

I looked into managing tslint-rules in a monorepo style by adding settings in Angular’s angular.json for ng, but it turned out to be somewhat Angular-dependent, and the tslint-rules don’t have enough volume to justify being structured as a separate repository. While using @nrwl/workspace:run-commands(example) would allow the desired command(tsc build) to run directly(because it has to be done this way to build something that's not Angular), it requires installing a separate package(@nrwl/workspace), I also had to find a package that's compatible with Angular 9, and I was concerned about whether this was the right approach. Therefore, tslint-rules is handled separately during linting in package.json, and its scope is also separated in lint-staged.

I thought ng lint --fix wasn’t supported in the older version of ng, but I confirmed that it works and have used it to set up the lint script.

tbonelee
tbonelee previously approved these changes Sep 6, 2025
Copy link
Contributor

@tbonelee tbonelee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM👍 Related CI job has also passwed.

@tbonelee tbonelee merged commit 7110bed into apache:master Sep 6, 2025
16 of 18 checks passed
tbonelee pushed a commit that referenced this pull request Sep 6, 2025
…rojects folder

### What is this PR for?
> This PR for **zeppeling-web-angular**

In `npm run lint`, `projects folder` is linted using ng, but `prettier` only checks the `src folder`. In `lint-staged`, neither lint nor `prettier` checks the `projects folder`. This seems to need fixing.

Because `ng lint --fix` is not supported, I added a `lint:fix` script to run `tslint` with the `--fix` option.

Since src and projects folders need to reference different tslint configs, I added the corresponding option to `lint-staged`.

### What type of PR is it?

Bug Fix
Improvement

### Todos

### What is the Jira issue?
* [[ZEPPELIN-6305](https://issues.apache.org/jira/browse/ZEPPELIN-6305)]

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the license files need to update? N
* Is there breaking changes for older versions? N
* Does this needs documentation? N

Closes #5054 from dididy/fix/ZEPPELIN-6305.

Signed-off-by: ChanHo Lee <[email protected]>
(cherry picked from commit 7110bed)
Signed-off-by: ChanHo Lee <[email protected]>
@tbonelee
Copy link
Contributor

tbonelee commented Sep 6, 2025

Merged into master and branch-0.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants