-
Notifications
You must be signed in to change notification settings - Fork 180
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
Create-ApiReview
honors PackageProperty files
#9412
base: main
Are you sure you want to change the base?
Conversation
…re present in the artifact list
The following pipelines have been queued for testing: |
$result = ProcessPackage -packageName $artifact.name | ||
$responses[$artifact.name] = $result | ||
$propFiles = Get-ChildItem -Path $ConfigFileDir -Recurse -Filter "*.json" | ||
$artifactPackages = $ArtifactList | ForEach-Object { $_["name"] } |
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.
Shouldn't this be $_.name
or is this a hashtable object?
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's a hashtable object, both will actually work here.
$artifact = Get-Content $propFile.FullName -Raw | ConvertFrom-Json -AsHashtable | ||
if ($artifactPackages -contains $propFile.Name.Replace(".json", "")) | ||
{ | ||
Write-Host "Processing $($artifact.ArtifactName)" |
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.
Isn't the artifact name the same as the file or are their cases where they are different? If they are the same, we probably can avoid reading the json file. If they aren't the same we probably should use the artifactname in your condition instead of the file name.
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.
$pkgInfo.ArtifactName
== <package-json-file-name>
everywhere I'm aware of. I'll add comments for clarity and double check, but I'm fairly certain.
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.
couple questions but otherwise looks good
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 good
See title.
@praveenkuttappan Detect-Api-Changes explicitly walks the prop files to begin with. So we're already covered there.
Pardon the other spacing updates. My
powershell
vscode extension is aggressive 👍