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.

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

npm/qsharp - Make languageFeatures an optional parameter again #2183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

minestarks
Copy link
Member

@minestarks minestarks commented Feb 14, 2025

Revert breaking change for QCOM. I don't recall the specific reason we had to make this required - I think it was an accident that snuck in when we made the languageFeatures a required method in OTHER method signatures in this file (#2078)

Revert breaking change for QCOM. I don't recall the specific reason we had to make this required - I think it was an accident that snuck in when we made the `languageFeatures` a required method in OTHER method signatures in this file.
@minestarks minestarks marked this pull request as ready for review February 24, 2025 23:12
@billti
Copy link
Member

billti commented Feb 26, 2025

The quoted PR where this broke seems like it was a deliberate change... but I don't see where this revert would break anything. If you've tested and looks good, then that's good with me.

@minestarks
Copy link
Member Author

minestarks commented Feb 26, 2025

Just going over the details for my own sanity: There were a few other arguments also called languageFeatures, that were changed to be required arguments, and those were deliberate, but this one must have got lumped in by accident. @idavis and I had discussed and decided it was ok to make languageFeatures optional on the getAst and getHir methods (discussed in this comment) This was not breaking since QCOM doesn't use those methods. What I missed in the final revision is that there was another languageFeatures property on ProgramConfig that was made required, and that was the breaking change. 😭

It ended up being nbd (QCOM has long been fixed) - but I wanted to get this PR out for hygiene. It's just a better API to leave that property optional.

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