Skip to content
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.

Sign up for GitHub

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

Jump to bottom

http-client-java, eng, configure deprecation lint rule #4670

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

http-client-java, eng, configure deprecation lint rule #4670

wants to merge 5 commits into from

Conversation

haolingdong-msft
Copy link
Member

No description provided.

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:java Issue for the Java client emitter: @typespec/http-client-java label Oct 10, 2024
@azure-sdk
Copy link
Collaborator

No changes needing a change description found.

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

haolingdong-msft
haolingdong-msft commented Oct 10, 2024
View reviewed changes
eslint.config.js Outdated
@@ -72,10 +72,10 @@ const allFilesConfig = tsEslint.config({
*/
export function getTypeScriptProjectRules(root) {
return tsEslint.config({
files: ["**/packages/*/src/**/*.ts", "**/packages/*/src/**/*.tsx"],
files: ["**/packages/*/src/**/*.ts", "**/packages/*/src/**/*.tsx", "**/packages/http-client-java/emitter/src/**/*.ts"],
Copy link
Member Author

@haolingdong-msft haolingdong-msft Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

lint job throws below error when running pnpm run lint:
https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206

Run pnpm run lint

> @typespec/[email protected] lint /home/runner/work/typespec/typespec
> eslint . --max-warnings=0


Oops! Something went wrong! :(

ESLint: 9.11.1

RangeError: Maximum call stack size exceeded
Occurred while linting /home/runner/work/typespec/typespec/packages/http-client-java/emitter/src/common/client.ts:31
Rule: "@typescript-eslint/no-deprecated"
    at findBaseOfDeclaration (/home/runner/work/typespec/typespec/node_modules/.pnpm/typescript@5.[6](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:7).2/node_modules/typescript/lib/typescript.js:149319:31)
    at getJsDocTagsOfDeclarations (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:149282:29)
    at SymbolObject.getContextualJsDocTags (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:149109:44)
    at /home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:14928[7](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:8):27
    at /home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:14932[8](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:9):21
    at firstDefined (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:2307:20)
    at findBaseOfDeclaration (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:14[9](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:10)324:[10](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:11))
    at getJsDocTagsOfDeclarations (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:[14](https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206#step:6:15)9282:29)
    at SymbolObject.getContextualJsDocTags (/home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:149109:44)
    at /home/runner/work/typespec/typespec/node_modules/.pnpm/[email protected]/node_modules/typescript/lib/typescript.js:149287:27
 ELIFECYCLE  Command failed with exit code 2.v

Copy link
Member Author

@haolingdong-msft haolingdong-msft Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But if we only include files under src/ folder, lint will pass. I guess it's because eslint can't handle too much levels of folder hierarchy.
see this commit: 82cfd66
and lint job: https://github.com/microsoft/typespec/actions/runs/11267081108/job/31331611206
This lint job passed.

Copy link
Member Author

@haolingdong-msft haolingdong-msft Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @timotheeguerin May I know your suggestion on this? Basically we want to add deprecation lint rules for all ts files under http-client-java/emitter/, but when we include those ts files to eslint.config.js, pnpm run lint will throw Maximum call stack size exceeded error, seems due to eslint can't handle too much levels of folder hierarchy. If I only include files directly under http-client-java/emitter, pnpm run lint is able to run.
/cc @srnagar

Copy link
Member

Choose a reason for hiding this comment

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

yeah the typescript eslint dependency is struggling with large projects

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @timotheeguerin for reply! Do you have suggestions on how to add deprecation lint rules for http-client-java's files (I guess other language's emitters also need this)?

@haolingdong-msft haolingdong-msft changed the title http-client-java, configure deprecation lint rule http-client-java, eng, configure deprecation lint rule Oct 10, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the stale Mark a PR that hasn't been recently updated and will be closed. label Nov 21, 2024
@microsoft-github-policy-service Microsoft GitHub Policy Service
Copy link
Contributor

Hi @haolingdong-msft. Your PR has had no update for 30 days and it is marked as a stale PR. If it is not updated within 30 days, the PR will automatically be closed. If you want to refresh the PR, please remove the stale label.

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

@timotheeguerin timotheeguerin timotheeguerin left review comments

@srnagar srnagar Awaiting requested review from srnagar srnagar will be requested when the pull request is marked ready for review srnagar is a code owner

@weidongxu-microsoft weidongxu-microsoft Awaiting requested review from weidongxu-microsoft weidongxu-microsoft will be requested when the pull request is marked ready for review weidongxu-microsoft is a code owner

@XiaofeiCao XiaofeiCao Awaiting requested review from XiaofeiCao XiaofeiCao will be requested when the pull request is marked ready for review XiaofeiCao is a code owner

@bterlson bterlson Awaiting requested review from bterlson bterlson will be requested when the pull request is marked ready for review bterlson is a code owner

@markcowl markcowl Awaiting requested review from markcowl markcowl will be requested when the pull request is marked ready for review markcowl is a code owner

@allenjzhang allenjzhang Awaiting requested review from allenjzhang allenjzhang will be requested when the pull request is marked ready for review allenjzhang is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
emitter:client:java Issue for the Java client emitter: @typespec/http-client-java stale Mark a PR that hasn't been recently updated and will be closed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haolingdong-msft @azure-sdk @timotheeguerin