Skip to content

Disable generation if enabling noEmit flag#2948

Merged
MaryGao merged 2 commits intomainfrom
fix-noEmit-flag-issue
Dec 12, 2024
Merged

Disable generation if enabling noEmit flag#2948
MaryGao merged 2 commits intomainfrom
fix-noEmit-flag-issue

Conversation

@MaryGao
Copy link
Copy Markdown
Member

@MaryGao MaryGao commented Dec 11, 2024

Our emitter honored --no-emit flag but in the middle of generation, this would lead to some actions taken accidentally like clean up the generation folder etc. It's better to disable generation if enabling noEmit flag at the begining of emitter no matter it is RLC or modular.

This could help fix the files automatical deletion issue in smoke test when typespec code extension enabled.

@MaryGao MaryGao changed the title Disable generation if enabling noEmit flag Disable generation if enabling noEmit flag at the begining Dec 11, 2024
@MaryGao MaryGao marked this pull request as ready for review December 11, 2024 09:38
@MaryGao MaryGao changed the title Disable generation if enabling noEmit flag at the begining Disable generation if enabling noEmit flag Dec 11, 2024
@qiaozha
Copy link
Copy Markdown
Member

qiaozha commented Dec 11, 2024

⁠Can we special case the clearSrcFolder only to resolve this ? I feel like no-emit doesn't mean to do nothing.

@MaryGao
Copy link
Copy Markdown
Member Author

MaryGao commented Dec 11, 2024

⁠Can we special case the clearSrcFolder only to resolve this ? I feel like no-emit doesn't mean to do nothing.

@qiaozha could you help me understand what is the value we could get from if we don't plan to emit codes?

I learned from emitter template and other emitters e.g csharp that to skip generation at the begining.

Another concern is for performance, during enabling code extension, typespec server side would automatically emit all configed specs with --no-emit flag, since our emitter has performance issue it would occupy the local machine CPU. So personally to make things simpler I prefer to skip at the begining if no much value provided.

{FE4CEAC3-FD4D-45D0-BE24-974D6A7E02AF}

@qiaozha
Copy link
Copy Markdown
Member

qiaozha commented Dec 11, 2024

did you also notice that the above template link is putting no-emit before emitFile? the same for openapi3 emitter https://github.com/microsoft/typespec/blob/main/packages/openapi3/src/openapi.ts#L264.

@MaryGao
Copy link
Copy Markdown
Member Author

MaryGao commented Dec 11, 2024

did you also notice that the above template link is putting no-emit before emitFile? the same for openapi3 emitter https://github.com/microsoft/typespec/blob/main/packages/openapi3/src/openapi.ts#L264.

we see both cases, directly returning at the begining like Java or .Net or returning just before the final emitting like openapi. it is more like emitter's owner decision. for long term i agreed that the latter one would be better because we could still verify the emitter's diagnostics even in --no-emit.

but in short term considering we have performance issue, I would prefer to directly return and do that once we fix this issue: #2879.

another thing I was thinking from typespec side it's better to differentiate two cases 1) I don't want to emit files but I want to run emitters to see what happened vs 2) I don't want to emit files & I am not interested in what will happen in that emitter. currently the --no-emit in typespec server would be the second case, because from customers of vs extension may not know emitter output.

Copy link
Copy Markdown
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

LGTM, since tcgc doesn't prepare the sdkPackage when it's noEmit, I don't think our own code logic would work as expected with the current implementation either.

@MaryGao MaryGao merged commit e265539 into main Dec 12, 2024
@MaryGao MaryGao deleted the fix-noEmit-flag-issue branch December 12, 2024 05:24
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