-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix: Renamed spec files and incorrect cSpell ignores #12584
Conversation
Hi, @nschonni Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected] |
Swagger Validation Report
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
Thank you for your contribution nschonni! We will review the pull request and get back to you soon. |
6caab09
to
f8d0f05
Compare
cSpell.json
Outdated
@@ -716,9 +716,8 @@ | |||
] | |||
}, | |||
{ | |||
"filename": "**/specification/servicebus/resource-manager/Microsoft.ServiceBus/preview/2018-01-01-preview/servicebus-preview.json", | |||
"filename": "**/specification/servicebus/resource-manager/Microsoft.ServiceBus/preview/2018-01-01-preview/NetworkRuleSet-preview.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file doesn't exist in master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, looks like it got renamed again in #11743
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is fixed now, so I removed it and added some related missing file-specific overrides that had gotten added to the root dictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a little test script and cleaned out some other renamed files
const fs = require('fs');
const cspell = JSON.parse(fs.readFileSync('cSpell.json', 'utf-8'));
cspell.overrides.forEach(element => {
let filename = element.filename.substring(3);
try {
let content = fs.readFileSync(filename, 'utf-8')
element.words.forEach(word => {
if (content.indexOf(word) === -1) {
console.warn(`BAD: ${filename}\n${word}`)
}
})
} catch (error) {
console.warn(`BAD: ${filename}\n${error}`)
}
});
6d131c9
to
30e3afa
Compare
8ad6f85
to
814bc66
Compare
814bc66
to
0798e92
Compare
0798e92
to
222e5ea
Compare
@raych1 added another missing "Regenerte" for the current failure on the main branch https://github.com/Azure/azure-rest-api-specs/runs/1765899989 |
40036d8
to
771fe35
Compare
771fe35
to
0bc0769
Compare
Hi, @nschonni. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
0bc0769
to
69889bf
Compare
privatepreview | ||
projectable | ||
propogation | ||
Protectable | ||
ProviderHub | ||
Providerhub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changed the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It had been added with both casings. CamelCase is already treated as separate words by cSpell
@@ -1518,7 +1530,6 @@ RSNULL | |||
rstrnt | |||
RTMP | |||
RTSP | |||
Ruleproperties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this removing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to a file level suppression again
@@ -1589,7 +1600,6 @@ sessionids | |||
sessionstate | |||
SETACL | |||
SETEXPIRY | |||
Setget | |||
setissuers | |||
SETOWNER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File level suppression
@@ -1971,7 +1982,6 @@ vmimage | |||
vmname | |||
vmotion | |||
VMQS | |||
vmsizes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File level suppression
@@ -2070,29 +2080,3 @@ ziplist | |||
Zoho | |||
zoneinfo | |||
zset | |||
tpgs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these words in this file any more. Why do we remove them?
tpgs
ProviderHub
regionality
Resource
metdata
Exprired
saskey
vmsize
FSLogix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some were unfixed typos moved to file level suppression in cspell.json
, duplicates of words that were already in the file in the right sort location, CamelCase words, that cSpell already treats as separate words, or correctly spelled words like "Resource" that shouldn't be in this overrides dictionary
@@ -333,6 +255,12 @@ | |||
"Regenerte" | |||
] | |||
}, | |||
{ | |||
"filename": "**/specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.1/storage.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add for other keyvault versions too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were the only ones with the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nschonni , sorry for the late reply. Can you have a look at my comments?
69889bf
to
4b4c04b
Compare
@@ -304,9 +309,11 @@ changedSince | |||
changepoint | |||
changestate | |||
CHECKACCESS | |||
Checkfeature | |||
CheckfeatureSupport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this one is needed since "Checkfeature" is already in, and "Support" would be treated as a separate word because it is camelCase.
It's possible "Checkfeature" should be moved to a file level suppression and fixed in the spec though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nschonni for the reply. Can you run the spell check locally for all the specs and check the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that over in #11474, but due to the size I don't think it's going to land till smaller pieces can be landed to shrink it
@nschonni , could you please resolve the conflicts so that I can merge this PR? Thanks. |
4b4c04b
to
3e6001d
Compare
3e6001d
to
a308d12
Compare
@nschonni , can you resolve the conflicts again? Thanks. |
a308d12
to
9ebbef1
Compare
@raych1 rebased again |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.