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

Migration script: warn and do not migrate #3046

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Jan 28, 2021

Related to #3044.

Here is the behaviour

  • on yt 4, yt.toml and ytcfg are present: raise a warning
  • on yt 4, yt.toml does not exist, ytcfg exists: raise another warning and do not migrate
  • in any case, a yt.toml will be created (see Unnecessary config file creation #3045)

@cphyc cphyc changed the title Get rid of SystemExit Get rid of SystemExit Jan 28, 2021
@cphyc cphyc changed the title Get rid of SystemExit Migration script: warn and do not migrate Jan 28, 2021
@cphyc cphyc added enhancement Making something better proposal Proposals for enhancements, changes, etc labels Jan 28, 2021
@cphyc cphyc added the yt-4.0 feature targeted for the yt-4.0 release label Feb 19, 2021
@munkm munkm self-requested a review February 25, 2021 20:50
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Just a remark: it is not clear from the warning message wether the deprecated config file is being loaded or ignored in the CLI call case. Otherwise, LGTM

removal="4.1.0",
)
raise SystemExit
issue_deprecation_warning(
Copy link
Member Author

@cphyc cphyc Feb 26, 2021

Choose a reason for hiding this comment

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

The reason we can simplify this is that there is no circular import issue any more AND no SystemExit being raised. The slight caveat is that now yt config migrate will advise users ... to use yt config migrate!

$ yt config migrate
[…]/yt/yt/utilities/logger.py:4: VisibleDeprecationWarning: The configuration file […]/.config/yt/ytrc is deprecated. Please migrate your config to […]/.config/yt/yt.toml by running: 'yt config migrate'
Deprecated since v4.0.0 . This feature will be removed in v4.1.0
  from yt.config import ytcfg
INFO: using configuration file: […]/.config/yt/yt.toml.
Writing a new config file to: […]/.config/yt/yt.toml
Backing up the old config file: […]/.config/yt/ytrc.bak

Note that this shouldn't be an issue as an subsequent calls to yt config migrate won't show the warning and won't do anything (as the config has already been migrated).

@munkm munkm merged commit 72f4df7 into yt-project:main Mar 4, 2021
@cphyc cphyc deleted the improve-migration-script-no-error branch March 4, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better proposal Proposals for enhancements, changes, etc yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants