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.

Sign up for GitHub

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

Jump to bottom

refactor: modifying subprocess calls and removing try except continue statements #3474

Merged
merged 92 commits into from
Oct 23, 2024
Merged

refactor: modifying subprocess calls and removing try except continue statements #3474

merged 92 commits into from
Oct 23, 2024

Conversation

clatapie
Copy link
Contributor

@clatapie clatapie commented Oct 9, 2024 •
edited
Loading

Description

This PR:

  • modifies subprocess calls. subprocess is now used with shell=False
  • removes all the try except continue statements by adding warnings when needed
  • changes the urllib.request calls into requests ones

Checklist

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the maintenance General maintenance of the repo (libraries, cicd, etc) label Oct 9, 2024
@github-actions github-actions bot added maintenance General maintenance of the repo (libraries, cicd, etc) and removed maintenance General maintenance of the repo (libraries, cicd, etc) labels Oct 9, 2024
@github-actions github-actions bot added maintenance General maintenance of the repo (libraries, cicd, etc) and removed maintenance General maintenance of the repo (libraries, cicd, etc) labels Oct 9, 2024
@clatapie clatapie changed the title maint: PyMAPDL general maintenance refactor: PyMAPDL general maintenance Oct 9, 2024
@github-actions github-actions bot added enhancement Improve any current implemented feature and removed maintenance General maintenance of the repo (libraries, cicd, etc) labels Oct 9, 2024
@clatapie clatapie self-assigned this Oct 9, 2024
@clatapie
Copy link
Contributor Author

clatapie commented Oct 9, 2024

The errors probably come from the shell=False changes. However, as the other changes were not tested prior modifying the subprocess calls, the next commits will make sure that the rest of the code is correct.

germa89 and others added 16 commits October 16, 2024 17:03
@germa89
test: fix tests
c751eae
@clatapie
test: reverting change in f'"{exec_file}"'
3acc746
@clatapie
fix: exec_file errors
4e059a6
@clatapie
Merge branch 'main' into maint/pymapdl_maintenance
027bfa5
@germa89
chore: merge remote-tracking branch 'origin/main' into maint/pymapdl_…
41f716e 
…maintenance
@clatapie
Merge branch 'main' into maint/pymapdl_maintenance
cbbae17
@clatapie
review: applying @germa89's suggestions
09a9a25
@clatapie @germa89
Apply suggestions from code review
7b846be 
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
@clatapie
review: applying @germa89's suggestions
25a9db4
@clatapie @germa89
Update src/ansys/mapdl/core/mapdl_core.py
19cbea9 
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
@clatapie
review: applying @germa89's suggestions - 2
2235a19
@clatapie
fux: using self._log.debug
4e0688c
@clatapie
fix: test_examples.py
d845e24
@clatapie
fix: generate_mapdl_launch_command
7b857d3
@clatapie
fix: _checkout_license
8304fd4
@clatapie
fix: launch_grpc for Linux
52c7b8c
RobPasMue
RobPasMue approved these changes Oct 22, 2024
View reviewed changes
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.

Overall LGTM - just left a few comments

src/ansys/mapdl/core/examples/downloads.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/launcher.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/launcher.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/launcher.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_console.py Outdated Show resolved Hide resolved
germa89
germa89 approved these changes Oct 22, 2024
View reviewed changes
src/ansys/mapdl/core/examples/downloads.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/examples/downloads.py Show resolved Hide resolved
src/ansys/mapdl/core/launcher.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_core.py Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_grpc.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/pool.py Show resolved Hide resolved
src/ansys/mapdl/core/xpl.py Outdated Show resolved Hide resolved
germa89
germa89 approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@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.

I think this is mostly done, but some small changes are needed.

@clatapie clatapie requested a review from germa89 October 22, 2024 16:14
@clatapie clatapie enabled auto-merge (squash) October 23, 2024 07:32
@clatapie clatapie merged commit e4cc11e into main Oct 23, 2024
41 of 44 checks passed
@clatapie clatapie deleted the maint/pymapdl_maintenance branch October 23, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobPasMue RobPasMue RobPasMue approved these changes

@pyansys-ci-bot pyansys-ci-bot Awaiting requested review from pyansys-ci-bot pyansys-ci-bot is a code owner automatically assigned from ansys/pymapdl-maintainers

@germa89 germa89 Awaiting requested review from germa89

Assignees

@clatapie clatapie

Labels
enhancement Improve any current implemented feature maintenance General maintenance of the repo (libraries, cicd, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clatapie @RobPasMue @germa89 @pyansys-ci-bot