Skip to content

Conversation

@andrecsilva
Copy link
Contributor

No description provided.

@andrecsilva andrecsilva marked this pull request as ready for review July 4, 2025 14:20
try:
changeset: v2ChangeSet = next(cs for cs in result.changeset if cs.fixedFindings)
except StopIteration:
logger.debug("No fixedFinding in the given Result")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StopIteration could be raised from either result.changeset being empty or it's not empty but none of the changesets have a. fixefinding so the debug statement isn't quite accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll fix that

generation_metadata = GenerationMetadata(
strategy=Strategy.ai if changeset.ai else Strategy.deterministic,
ai=from_v2_aimetadata(changeset.ai) if changeset.ai else None,
provisional=False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this function always be called from non-provisional contexts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No clue, not sure why this flag exists in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provisional=True is "magic"... maybe we no longer make this distinction ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be derived from the v2 changeset the same way as these other metadata fields.

Suggested change
provisional=False,
provisional=changeset.provisional,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance that for our purposes it makes more sense to pass in these metadata fields as parameters to this new function? For the use case I'm thinking of, I'd argue that the answer is yes.

generation_metadata = GenerationMetadata(
strategy=Strategy.ai if changeset.ai else Strategy.deterministic,
ai=from_v2_aimetadata(changeset.ai) if changeset.ai else None,
provisional=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be derived from the v2 changeset the same way as these other metadata fields.

Suggested change
provisional=False,
provisional=changeset.provisional,

"""
# Find the changeset with a fixedFinding
try:
changeset: v2ChangeSet = next(cs for cs in result.changeset if cs.fixedFindings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed findings can belong to individual Change entries of a change set as well, so you need to check there too, both here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are usually replicated. That is, the changeset fixedFindings should always include the ones contained in the changes.

I'll make it check each individual change in any case.

generation_metadata = GenerationMetadata(
strategy=Strategy.ai if changeset.ai else Strategy.deterministic,
ai=from_v2_aimetadata(changeset.ai) if changeset.ai else None,
provisional=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance that for our purposes it makes more sense to pass in these metadata fields as parameters to this new function? For the use case I'm thinking of, I'd argue that the answer is yes.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2025

@andrecsilva andrecsilva requested a review from drdavella July 7, 2025 18:05
@andrecsilva andrecsilva merged commit eb501e6 into main Jul 8, 2025
13 checks passed
@andrecsilva andrecsilva deleted the ISS-3887/fix-v2-to-v3-with-mult-changesets branch July 8, 2025 10:14
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.

3 participants