Skip to content

Return False instead of raising an error if graph validation fails.#292

Closed
DaylonSrinivasan wants to merge 1 commit intofacebookresearch:mainfrom
DaylonSrinivasan:export-D79759829
Closed

Return False instead of raising an error if graph validation fails.#292
DaylonSrinivasan wants to merge 1 commit intofacebookresearch:mainfrom
DaylonSrinivasan:export-D79759829

Conversation

@DaylonSrinivasan
Copy link

Differential Revision: D79759829

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 6, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79759829

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79759829

…acebookresearch#292)

Summary:

The main CriticalPathAnalysis.critical_path_analysis method has the following contract:

  Returns: Tuple[CPGraph, bool] a pair of CPGraph object and a success or
      fail boolean value. True indicates that the critical path analysis
      algorithm succeeded.

But prior to this change, when graph validation failed, it actually raised a ValueError instead, while other errors (such at network failures) returned False. Mixed error handling (exceptions vs returning false) is an anti-pattern; this change makes it so that we consistently return False instead.

Reviewed By: seansundor

Differential Revision: D79759829
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D79759829

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in b75f84d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants