fix: inquire returning short env vars - #4074
Merged
Merged
Conversation
Contributor
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Contributor
Reviewer's GuideThis PR adds a warning in the Sequence diagram for Mapdl.inquire with long environment variable responsesequenceDiagram
participant User
participant Mapdl
participant warnings
User->>Mapdl: inquire(strarray, func, ...)
Mapdl->>Mapdl: response = ...
Mapdl->>Mapdl: if len(response) >= 248
Mapdl->>warnings: warn("Response might have been trimmed...")
Mapdl-->>User: response
Class diagram for updated Mapdl.inquire methodclassDiagram
class Mapdl {
+inquire(strarray, func, arg1, arg2, **kwargs)
}
Mapdl : +inquire() now warns if response >= 248 chars
Mapdl : +inquire() suggests alternative method for env vars
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @germa89 - I've reviewed your changes - here's some feedback:
- Extract the hard‐coded 248-character limit into a named constant (e.g. MAX_MAPDL_STRING_LENGTH) to improve clarity and avoid magic numbers.
- Adjust the warning condition to trigger only when the response length exceeds 248 characters (i.e. > 248) to prevent false positives on exact-length responses.
- Include the queried environment variable name in the warning message so it’s clear which variable may have been truncated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the hard‐coded 248-character limit into a named constant (e.g. MAX_MAPDL_STRING_LENGTH) to improve clarity and avoid magic numbers.
- Adjust the warning condition to trigger only when the response length exceeds 248 characters (i.e. > 248) to prevent false positives on exact-length responses.
- Include the queried environment variable name in the warning message so it’s clear which variable may have been truncated.
## Individual Comments
### Comment 1
<location> `src/ansys/mapdl/core/mapdl_extended.py:1465` </location>
<code_context>
response = response.split("=")[1].strip()
+ if len(response) >= 248:
+ warnings.warn(
+ "Response might have been trimmed to 248 characters because of "
+ "MAPDL string limitations. "
+ "Check the output of 'mapdl.inquire' carefully. "
+ "Alternatively, you can use 'mapdl.sys('printenv') to obtain "
+ "the environment variables on Linux."
+ )
+
</code_context>
<issue_to_address>
The warning message could clarify that the response is truncated, not just trimmed.
Consider updating the message to: "Response truncated to 248 characters due to MAPDL string limitations" for greater clarity.
</issue_to_address>
### Comment 2
<location> `tests/test_mapdl.py:1259` </location>
<code_context>
+ pytest.skip(f"Environment variable {envvar} not found")
+
+ value = env_vars[envvar]
+ if envvar in ["PATH", "LD_LIBRARY_PATH"] and len(value) > 248:
+ # MAPDL warns about long environment variables because it trims them to
+ # 248
+ with pytest.warns(UserWarning):
+ assert mapdl.inquire("", "ENV", envvar, 0) in value
+
else:
</code_context>
<issue_to_address>
Assertion could be more precise for trimmed environment variables.
Consider asserting that the returned value equals the first 248 characters of the environment variable to precisely verify the documented trimming behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
with pytest.warns(UserWarning):
assert mapdl.inquire("", "ENV", envvar, 0) in value
=======
with pytest.warns(UserWarning):
assert mapdl.inquire("", "ENV", envvar, 0) == value[:248]
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a warning when inquire returns a truncated string (≥248 chars) due to MAPDL limitations and expands/refactors tests to cover APDL, environment variables, titles, job names, file existence, and non‐interactive modes.
- Warn in
mapdl.inquireif the response may have been trimmed to 248 chars. - Rename and split existing inquire test into multiple focused tests.
- Add parametrized tests for environment variables (with warning on long values), title, jobname, exist checks, and non‐interactive behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_mapdl.py | Renamed test_inquire, added test_inquire_env, refactored other inquire tests |
| src/ansys/mapdl/core/mapdl_extended.py | Inserted a warning when response length ≥ 248 characters |
Comments suppressed due to low confidence (1)
tests/test_mapdl.py:1265
- The
else:here is indented by 6 spaces but should match the 4-space indent of its correspondingifintest_inquire_env, otherwise this will cause a syntax error.
else:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4074 +/- ##
==========================================
- Coverage 91.82% 91.80% -0.03%
==========================================
Files 187 187
Lines 14993 15023 +30
==========================================
+ Hits 13768 13792 +24
- Misses 1225 1231 +6 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
germa89
commented
Jul 4, 2025
Collaborator
Author
|
@pyansys-ci-bot LGTM. |
germa89
enabled auto-merge (squash)
July 4, 2025 18:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Due to MAPDL strings being limited to 248 chars, env vars which content is longer than that are limited to said length.
We cannot avoid it, but we can raise a warning and propose and alternative method.
Issue linked
Close #4072
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)Summary by Sourcery
Warn on trimmed environment variable responses in inquire and provide a printenv alternative, while significantly expanding and restructuring inquire tests into discrete, focused cases.
Enhancements:
Tests: