Skip to content

Conversation

@alexis-opolka
Copy link
Collaborator

@alexis-opolka alexis-opolka commented Mar 11, 2025

This PR is the result of the discussions in #938.

It adds the following:

  • Two new info commands ( default-cheats-path and default-config-path ) supposed to take over from the old info commands ( cheats-path and config-path )

  • Introduces a deprecation notice on the old info commands

  • Adds a reference inside the configuration on where it has been loaded from

  • Adds some minor debug expressions

  • Standardizes the way the functions retrieve the paths for each platform without introducing a breaking change (normally)

  • (EDIT) It updates the documentation accordingly

Fixes: #851

@alexis-opolka alexis-opolka marked this pull request as ready for review March 11, 2025 23:46
Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
@denisidoro
Copy link
Owner

Great! Could you please just fix the unneeded late initialization lint errors?

@kit494way
Copy link
Contributor

@alexis-opolka Is this ready for review? Is it intentional that the get_source method is implemented but not used?

@alexis-opolka
Copy link
Collaborator Author

Great! Could you please just fix the unneeded late initialization lint errors?

@denisidoro I should be able to do that next weekend.

@alexis-opolka Is this ready for review? Is it intentional that the get_source method is implemented but not used?

@kit494way , The get_source method went unnoticed through my cherry picks so it wasn't intentional to keep it inside this PR but it can be left for later when we'll introduce the config subcommands. I can remove it if we don't want to keep an unused method until we introduce them though.

@kit494way
Copy link
Contributor

I think it is better to include get_source in the pull request of the code that will actually use it.

@alexis-opolka
Copy link
Collaborator Author

@kit494way

What I was trying to do was to get where it has been loaded from on a logical level and not on a technical level.
I want us to be able to deduct easily where issues are from regarding the configuration and that means being able to know at runtime what type of configuration is applied.
I don't think I should put the configuration value inside the source attribute since we can already debug it and we intend to introduce the config subcommands. Now it seems pretty useless to have a duplicata of values.

What I can propose is to remove the configuration values from cfg.source and only put the logical source, that will give us something like this:

Logical origin cfg.source value
default value (aka. built-in) DEFAULT / BUILT-IN
default config file (YAML) YAML_CONFIG_FILE
Environment Variable with YAML (NAVI_CONFIG_YAML) ENV_NAVI_CONFIG_YAML
Environment Variable pointing to file (NAVI_CONFIG) ENV_NAVI_CONFIG

We can then get the real values since we know where to search.

This way also solves the issue I had on how to print the value.

@kit494way
Copy link
Contributor

What I can propose is to remove the configuration values from cfg.source and only put the logical source

I'm OK with that.

For the default config file, cfg.source value might be better as DEFAULT_CONFIG_FILE rather than YAML_CONFIG_FILE.

@alexis-opolka
Copy link
Collaborator Author

Another review would be appreciated in case I missed something otherwise I think this PR should be good to be merged.

@alexis-opolka
Copy link
Collaborator Author

I'll introduce the documentation on another PR, after #943 .

We now expose correctly the info subcommands and remove the possible breaking change with the use of `cheats-path` and `config-path` in scripts

Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
@alexis-opolka alexis-opolka mentioned this pull request Apr 7, 2025
Copy link
Owner

@denisidoro denisidoro left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge it.

@alexis-opolka alexis-opolka merged commit 9a6fae8 into denisidoro:master Apr 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please make navi info better

3 participants