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

Standardizing Windows user data path #44

Merged
merged 4 commits into from
May 25, 2023
Merged

Conversation

raph-luc
Copy link
Member

@raph-luc raph-luc commented May 24, 2023

Minor change

First described here ansys/pyfluent#1588, and later ansys/pyprimemesh#484

PRs: ansys/pyfluent#1615, ansys/pymapdl#2064, ansys/pyprimemesh#485

  1. Changes SETTINGS_DIR from user_data_dir("ansys_tools_path") which resolved to %LocalAppData%\ansys_tools_path\ansys_tools_path on Windows, to user_data_dir(appname="ansys_tools_path", appauthor="Ansys") which resolves to %LocalAppData%\Ansys\ansys_tools_path, same naming scheme as in PyFluent and the default Ansys Fluent Windows installation already creates the %LocalAppData%\Ansys root folder.

    This does not change anything in Linux as appauthor is unused on Linux.
  2. Exposing SETTINGS_DIR so that it can be used as ansys.tools.path.SETTINGS_DIR rather than ansys.tools.path.path.SETTINGS_DIR
  3. Reworked the examples for functions save_mapdl_path and save_mechanical_path to reflect these changes (no changes to the actual functions)

Note/food for thought: when ansys.tools.path is first imported, regardless of what the user is going to do, SETTINGS_DIR folder is immediately created, even if it is not going to be used. I would think this is probably unnecessary and you might want to change this (could break CI/tests though).

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #44 (10ac1e3) into main (0eceeae) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #44   +/-   ##
=======================================
  Coverage   69.47%   69.47%           
=======================================
  Files           3        3           
  Lines         249      249           
=======================================
  Hits          173      173           
  Misses         76       76           

@raph-luc raph-luc requested review from germa89 and RobPasMue May 24, 2023 18:01
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 - But I'll wait for @germa89 to review

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. Just a typo.

src/ansys/tools/path/path.py Outdated Show resolved Hide resolved
@RobPasMue RobPasMue merged commit a56b4d7 into main May 25, 2023
@RobPasMue RobPasMue deleted the fix/windows_data_path branch May 25, 2023 10:32
@@ -570,15 +570,15 @@ def save_mapdl_path(exe_loc=None, allow_prompt=True) -> str:

Notes
-----
The configuration file location (``config.txt``) can be found in
``appdirs.user_data_dir("ansys_tools_path")``. For example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@germa89 Pre-existing - but this helpstring does not quite match the purpose of save_mapdl_path

@koubaa
Copy link
Collaborator

koubaa commented May 30, 2023

@raph-luc @germa89 I'm concerned with the lack of migration here. If the user configured pymapdl a certain way, by upgrading to a new ansys-tools-path, the config file would be read from somewhere else and their configuration is lost. This isn't what I'd expect from a minor version upgrade.

Was this already released?

@raph-luc
Copy link
Member Author

@koubaa That is a good point: no, I checked and this was not released with ansys-tools-path 0.2.4.

I am not very familiar with how pyMAPDL uses ansys-tools-path so I am probably not the right person to help you figure that out, but do you have any examples of important changes the user may have added to that config file that would be lost? Wondering how they would fail when the file is moved. I believe we should be able to add some workarounds for this.

@koubaa
Copy link
Collaborator

koubaa commented May 31, 2023

@raph-luc look for the test case called test_migration which is used to test migration from pymapdl's own configuration management system to ansys-tools-path. @germa89 could advise on which migrations would be necessary to handle

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