introduced change that fixes issue #91#128
Conversation
natalialuzuriaga
left a comment
natalialuzuriaga
left a comment
There was a problem hiding this comment.
Before approving, just wanted to ask about the current implementation choices and see whether other alternate approaches could be better suited. I also defer to Sachin though since he wrote the existing code!
| async function calculateMetaData( | ||
| existingCodeJSON?: CodeJSON | null, | ||
| ): Promise<Partial<CodeJSON>> { |
There was a problem hiding this comment.
It is a big change to add existingCodeJSON as an argument to this core function and wondering if this is necessary to do? Based on reviewing the code, I see an existing pattern where merging content between existingCodeJSON and partialCodeJSON occurs in the main.ts (I'll point to it in the next comment). Also, the getBasicInfo() function does very similar functionality as getVersion()where it calls the GitHub API to fetch data about the repo and eventually merges this data with existingCodeJSON
Just wanted to flag and see if we can follow patterns laid out in the existing code for consistency and evaluate whether adding the existingCodeJSON argument is truly necessary
There was a problem hiding this comment.
yeah i would agree here that we want to stick with our design patterns currently.
There was a problem hiding this comment.
Here and below shows the pattern of how we merge existingCodeJSON with partialCodeJSON. We can do the same here with version?
| async function getBasicInfo(): Promise<BasicRepoInfo> { | ||
| try { | ||
| const [repoData, languagesData] = await Promise.all([ | ||
| octokit.rest.repos.get({ owner, repo }), | ||
| octokit.rest.repos.listLanguages({ owner, repo }), | ||
| ]); |
There was a problem hiding this comment.
Offering another implementation approach: we could instead add the version functionality to getBasicInfo() since it also uses octokit. Does octokit.rest.repos.get({owner, repo}) receive data on the repo's release tags? If so, could be worth just adding version into this function. If not, then a separate function makes sense
|
Last thing: as specified in our OSPO Gen AI Policy that was just shipped, please fill out this information about Gen AI usage and add this to your PR description: AI Usage
If checked, please provide an explanation on how AI was used in the development of this pull request:
|
sachin-panayil
left a comment
sachin-panayil
left a comment
There was a problem hiding this comment.
the logic is great but following along with Natalias comment, we should stick with the design patterns unless truly necessary
3 participants
Summary
This PR automates populating the
versionfield incode.jsonfrom the latest GitHub release while preserving the existing merge pattern insrc/main.ts.What changed
existingCodeJSONfromcalculateMetaData()insrc/helper.ts.versionfallback/merge logic intosrc/main.ts, following the same pattern already used forfeedbackMechanismandSBOM.src/__tests__/unit/helper.test.tssrc/__tests__/unit/main.test.tsNotes
repos.get()does not include latest release data.main.tspreserves the existingversionfromcode.json.Validation
npx jest src/__tests__/unit/helper.test.ts src/__tests__/unit/main.test.tsAI Usage
If checked, please provide an explanation on how AI was used in the development of this pull request:
Description: Used ChatGPT (Codex) to refactor the PR to align with the existing project design patterns based on reviewer feedback.
Type of assistance:
Code generation
Documentation
Debugging
Testing
Refactoring
Other:
Scope of usage:
src/main.ts,src/helper.ts,src/__tests__/unit/helper.test.ts,src/__tests__/unit/main.test.tsAI System used:
ChatGPT
Claude
Gemini
GitHub Copilot
Level of modification: - [ ] As-is - [x] Modified - [ ] Used as inspiration
Prompts used: "Please refactor my code to address reviewer feedback on my PR... revert the function signature in helper.ts, move the merging logic to main.ts, and update the corresponding unit tests."