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

[2051] Config Files Audit #2084

Merged
merged 5 commits into from
Nov 16, 2023
Merged

Conversation

Rixxan
Copy link
Contributor

@Rixxan Rixxan commented Oct 19, 2023

Broken out of #2068

Description

This is a full-pass update to the core config files, containing a major SemVer-compliant logic overhaul to the files. These updates are intended to optimize existing code, update older Python 2 style code to more updated standards, and increase readability and compatibility.

Type of change

  • Rewrite of Existing Code

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Multiple install runs, clean installs, self-updates, and others made to confirm functionality, including under a variety of existing pre-install situations.
  • All PyTest tests pass
  • Tested by the EDMC Internal Alpha Group and initial bugs fixed.

@Rixxan Rixxan marked this pull request as draft October 19, 2023 23:47
@Rixxan
Copy link
Contributor Author

Rixxan commented Oct 19, 2023

Seems something is up with the Linux changes. Time to drill that one down...

@norohind
Copy link
Contributor

Greetings. I was just wondering why to change type hints for list, dict, type to their deprecated alternatives in typing module? I honestly thought today it's common to migrate from typing.<type> to types itself as they now support subscripting.

@Rixxan
Copy link
Contributor Author

Rixxan commented Oct 30, 2023

Hey @norohind thanks for pointing that out.

I'll be honest, my brain was still operating under the older Python 3.8 typing system. I'll look into the newer typing system and see if we're making an accidental reversion instead of a forwarding of the system.

@Rixxan
Copy link
Contributor Author

Rixxan commented Nov 10, 2023

As I can't actually get a working Linux instance up right now, rolling back the linux config checks as they are failing the tests. Only going to work on things I can actually test right now.

I can't adequately test this right now, so not touching it.
Thanks to @norohind for catching this one! I'm not up to date on my PEP585.
@Rixxan Rixxan marked this pull request as ready for review November 10, 2023 16:36
@Rixxan Rixxan self-assigned this Nov 10, 2023
@Rixxan Rixxan requested a review from C1701D November 10, 2023 16:36
@Rixxan Rixxan added the code cleanup Cleaning up code label Nov 10, 2023
@Rixxan Rixxan added this to the 5.10.0 milestone Nov 10, 2023
@Rixxan Rixxan linked an issue Nov 10, 2023 that may be closed by this pull request
Copy link
Member

@C1701D C1701D left a comment

Choose a reason for hiding this comment

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

Looks good re functionality, some small thing re readability in config/__init__.py though.
Additionally you're moving back to the deprecated Optional[] in two files.

config/__init__.py Outdated Show resolved Hide resolved
tests/config/_old_config.py Outdated Show resolved Hide resolved
tests/journal_lock.py/test_journal_lock.py Outdated Show resolved Hide resolved
And re-adds the missing but deprecated abstracts in config
@Rixxan
Copy link
Contributor Author

Rixxan commented Nov 16, 2023

Updated as indicated. Oldstyles replaced and documentation added in lieu of removal.

image

@C1701D C1701D merged commit fae7b32 into EDCD:develop Nov 16, 2023
1 check passed
@C1701D C1701D deleted the enhancement/2051/config branch November 16, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Cleaning up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants