Skip to content
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: add an exception if executable not found #184

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

dipinknair
Copy link
Contributor

Currently, if no executable found in default location when using get_mechanical_path , then we are returning None. This is causing issue when we call version_from_path where we give path as input. We do not have any check if path is None.

Alternately we can avoid this exception and return path as empty string (as it is) it is instead of None. If that is the case then version_from_path will take care of value error if path is empty string

@dipinknair dipinknair requested review from koubaa and germa89 May 6, 2024 21:39
@dipinknair dipinknair marked this pull request as ready for review May 7, 2024 13:58
@dipinknair dipinknair requested a review from RobPasMue May 7, 2024 13:59
@ansys ansys deleted a comment from RobPasMue May 9, 2024
Copy link
Collaborator

@koubaa koubaa left a comment

Choose a reason for hiding this comment

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

The message of the exception isn't actionable by the end user

@dipinknair dipinknair requested a review from koubaa May 13, 2024 18:34
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM - although we could actively check for None as well. But this is more than enough

@RobPasMue
Copy link
Member

We should do a new release after #193 - thanks @nmalsang - what's the status here? We could include this PR as well.

germa89
germa89 previously requested changes Jun 6, 2024
Copy link
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

LGTM. But I think the message could be more explicative.

src/ansys/tools/path/path.py Outdated Show resolved Hide resolved
@dipinknair dipinknair requested a review from germa89 June 11, 2024 14:43
@RobPasMue RobPasMue dismissed germa89’s stale review June 12, 2024 13:22

Changes addressed

@RobPasMue RobPasMue merged commit 5f606e7 into main Jun 12, 2024
23 checks passed
@RobPasMue RobPasMue deleted the fix/get_application_path branch June 12, 2024 13:22
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.

4 participants