[Search] Regenerate with 2024-03-01-Preview spec#28576
Conversation
fe1e6de to
658ac19
Compare
maorleger
left a comment
There was a problem hiding this comment.
Unfortunately wasn't able to complete reviewing, but there's a lot I like about it 😄
There are some concerns I have regarding the types and breaking changes - happy to discuss!
There was a problem hiding this comment.
Nit: can we move this to https://github.com/maorleger/azure-sdk-for-js/blob/main/.vscode/cspell.json and keep all the cspell in the same place?
There was a problem hiding this comment.
Sure, these actually belong in .vscode/cspell.json, so I'll move those over.
There was a problem hiding this comment.
Ignore me if these are all aliases to string, but if they are unions or some other narrower type this would be a breaking change?
I won't comment on every change here yet but we should not have breaking changes in a minor release so please do validate that 👍
There was a problem hiding this comment.
Some of these changes are just named string aliases, some of them are string literal unions where they're output-only parameters. But only purportedly. I mentioned this elsewhere, but it's probably not worth the maintenance burden to manually track down which extensible enums are output-only, and I'd have to think about whether or not it's worth making a Swagger parser to automate this.
There was a problem hiding this comment.
Like here, if these are input params then this would be a breaking change. If they are output only then I think it's fine
There was a problem hiding this comment.
I intended for only output parameters to have custom string literal unions, but I think I might be creating too much of a maintenance burden here. Without proper tooling to help determine what types are output-only, and to catch when an output only type is consumed in a later revision, it's probably too much effort to maintain this convenience.
We're hoping to iron out our extensible enum plans over the coming weeks. Until then, I'm thinking I'll revert these where I can. But there's a not insignificant chance that I've already introduced a string literal union where it doesn't belong in the last GA release. Lots of bugs to squash there.
There was a problem hiding this comment.
I've added a script to generate diffs against the upstream stable and main (preview) branches for the API Extractor output. Any unified diff GUI/TUI should be able to render those files (under the review dir), but any hunks that don't modify/remove any lines are omitted since they're ostensibly just new features. There are some spurious errors in there though (the difftool removes a line in one hunk and adds an identical line in another).
In the same commit, there's a script to generate string literal unions from TS files with enum declarations. Again, spurious errors, but nothing too difficult to manually check/correct.
Hopefully, these changes will make it a bit easier to assess where the API surface diverges and speeds up feature development moving forward. I'll need to make a runbook though; this is getting a bit too complex not to have documentation.
There was a problem hiding this comment.
Thanks, i'm starting to like the idea of linting our samples-dev files as well
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
c9cc71a to
5a1f38d
Compare
There was a problem hiding this comment.
Did these get moved or no longer needed? I guess why are they missing and is that expected?
There was a problem hiding this comment.
They were renamed to QueryAnswer and QueryCaption.
There was a problem hiding this comment.
did we mean to expose the pipeline here?
There was a problem hiding this comment.
Yes. The service is moving fast, so we're exposing the pipeline to allow customers to send raw requests with our HTTP pipeline. It's called out in the changelog.
There was a problem hiding this comment.
what is this for, and why not use JavaScript? 😈
There was a problem hiding this comment.
It's a small script to save me some time while hand-editing the typeDefinitions.ts test files. It just renames every const variable declaration with sequential strings. There's more that I intended to automate in that space, but I think I either lost some of that progress in a Codespace rebuild or otherwise shelved that for other priorities.
As for why it's Perl, I find that text file manipulation is a bit more idiomatic. perl -i modifies files in-place, so it's common to find Perl one-liners in the form perl -i -pe 's/foo/bar/msg' foo.txt (replaces all occurrences of foo with bar).
There was a problem hiding this comment.
FWIW we used to have a small .js build script for EventGrid that did some massaging of types. I think in general it'd be good to stick to JS/TS when possible so you can hand it off to other maintainers more easily.
d292cc6 to
0cf5c76
Compare
0cf5c76 to
0199a1d
Compare
0199a1d to
f65827e
Compare
ef34ff6 to
f55a190
Compare
f55a190 to
b45bd2a
Compare
b45bd2a to
b2c22d9
Compare
Packages impacted by this PR
@azure/search-documents
Issues associated with this PR
Resolves #28299, #28300
Describe the problem that is addressed by this PR
It might be helpful to review this commit-by-commit through 41c333828efbf08e977fee3948ceb587aff70c12
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists
See #28772 for more info on the last commit and title