Add the ability to set a config root folder in set_config #684

Firefly35 · 2021-12-09T09:32:22Z

Using set_config will search for the config filename from the current run directory of the application.
When the file is not found in this situation, it is searched for from the config_root_folder when it is set.
This pull request fixes the #682 issue.

…g set_config will search for the config filename from the current run directory of the application, when the file is not found in this situation, it is searched for from the config_root_folder when it is set.

…tibility.

phlptp · 2021-12-13T17:14:36Z

I am not sure I like this change. Adding paths here seems problematic to me. One thing I could see being useful is making support in the config_file for a ';' separated list. So it could check down the list until it found one and use that one. That would be a little more general support and still give the flexibility that this PR is proposing without adding a new function signature.

Firefly35 · 2021-12-13T20:49:46Z

In fact this functionality is a rather good solution for tools that host several configurations in a $home/.app/configs/... Calling the tool anywhere from the command line and switching config can - with the PR - just use the config "name" instead of providing full paths to every config file. It's a common pattern for command line tools to have a centralized configurations/profiles folder hosted in a "root" config folder. For the moment I maintain my own fork, but I'd prefer if in a way or another the functionality makes its way in CLI11. Why do you find adding a "standardized path" searched for when the config file was not found from its filepath is problematic ? Le lun. 13 déc. 2021 à 18:14, Philip Top ***@***.***> a écrit :

…

I am not sure I like this change. Adding paths here seems problematic to me. One thing I could see being useful is making support in the config_file for a ';' separated list. So it could check down the list until it found one and use that one. That would be a little more general support and still give the flexibility that this PR is proposing without adding a new function signature. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5Z2FC6DVZG5R43TMFXBKDUQYSYPANCNFSM5JV7CMYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

phlptp · 2021-12-13T21:27:49Z

Might be just that the applications I work on are all cross platform so there is no standardized locations and having an absolute path in a binary that could be installed in countless different ways leads to all sorts of problems. Or even a relative one would have to defined in a per operating system basis due to different path formats in use on different operating systems.

I suppose that might be an applications problem vs a CLI11 problem though.

Firefly35 · 2021-12-13T22:14:56Z

Indeed. And the root path built is per os (in fact the same on unixes including Linux and macos, but the home dir is built differently for windows). You can refer to the remaken application to see how I set it (see CmdOptions.cpp) here https://github.com/b-com-software-basis/remaken/tree/release/1.9.x Le lun. 13 déc. 2021 à 22:27, Philip Top ***@***.***> a écrit :

…

Might be just that the applications I work on are all cross platform so there is no standardized locations and having an absolute path in a binary that could be installed in countless different ways leads to all sorts of problems. Or even a relative one would have to defined in a per operating system basis due to different path formats in use on different operating systems. I suppose that might be an applications problem vs a CLI11 problem though. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5Z2FDTV32B4444HL6ZCUTUQZQN7ANCNFSM5JV7CMYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

phlptp · 2021-12-13T22:49:42Z

include/CLI/App.hpp

+                if(path_result == detail::path_type::nonexistent && !default_config_folder_.empty()) {
+                    // Try to find the config file from its name in default config folder
+                    std::string config_file_path = default_config_folder_;
+                    if(default_config_folder_.back() != '/' && default_config_folder_.back() != '\\') {


would this check be better in the set_config function to only do it once rather than every element of a loop?

phlptp · 2021-12-13T22:53:10Z

Wow, and I thought we had a lot of options in our applications.

I am fine with it, let @henryiii decide to merge it if he thinks it appropriate.
I know we went through a lot of revisions into CLI11 to not have to use a custom version and get the needed changes merged in, so thanks,

henryiii · 2022-01-28T05:12:32Z

set_config's arguments change in a non-backward compatible way and there are no tests. But IMO this is not needed at all; can't you just add a transform to the option that injects the default folder? I'd much rather build things out of existing components vs. add new API, especially new API that is a positional argument (so unreadable). A nice test + docs for how to do it would be great, though.

Firefly35 · 2022-01-28T10:55:07Z

Did you look deeply to the PR ? set_config has always the previous API signature, and there is an overload with the additionnal default_config_root_folder argument.
Moreover, I don't see how I can put a transform option as the config_root_folder is needed to figure out the root folder to search the config for while using set_config, AFAIK this happens before any option is parsed.

henryiii · 2022-01-28T16:20:01Z

there is an overload

I missed the overload. But that makes this potentially confusing, as well, since not only are there no names on the arguments in C++, the order is not stable either.

Moreover, I don't see how I can put a transform option as the config_root_folder is needed to figure out the root folder to search the config for while using set_config, AFAIK this happens before any option is parsed.

Maybe we should try to fix that instead? At least I think we should carefully consider trying that path first. The config option is already special, so I think we could build on that.

phlptp · 2022-01-28T16:47:08Z

I kind of like Henry's idea of enabling validators and transforms on the config option. That is probably an oversight on our part. Then I think a built in transform that checks paths and if necessary adds a default path might be quite useful in other contexts as well.

Firefly35 · 2022-01-28T17:13:11Z

I let you choose the best way to provide this feature in future CLI11 releases. For now, we live with the workaround I quickly implemented :). Concerning overloading in C++, there is IMHO no issue about arguments as the method signature differs on the number and kind of arguments. I don't think there can be any ambiguity between f(str,str,cstr,bool) and f(str,str,cstr,cstr,bool) ? Argument order would change if the calling convention was changed at compile time, but as CLI11 is header only the calling convention would be the same throughout the translation unit, so I don't get where the argument order would be an issue ? (provided the calling convention is the same in all libs/applications, but with different calling conventions, not only CLI11 would be impacted, every argument forwarding between the app and library would be impacted, no ?) Le ven. 28 janv. 2022 à 17:47, Philip Top ***@***.***> a écrit :

…

I kind of like Henry's idea of enabling validators and transforms on the config option. That is probably an oversight on our part. Then I think a built in transform that checks paths and if necessary adds a default path might be quite useful in other contexts as well. — Reply to this email directly, view it on GitHub <#684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5Z2FHQHPMOVXAQ3JAFWE3UYLCBRANCNFSM5JV7CMYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

phlptp · 2022-01-29T03:40:58Z

Just an update, transforms work fine on the config file option. They are executed before an options value gets used, which in the case of config is just before the file is processed, so a transform on the option from set_config works as expected and can be used.

henryiii · 2022-01-30T03:09:23Z

My worry was in teaching it / reading it; default arguments are easier, though they still require a user look them up, but different signatures require each signature to be documented, for readers to be familiar with both, and count parameters, etc. Not that it can't be handled by a compiler.

phlptp · 2022-01-31T23:09:06Z

I am going to close this as a similar capability was merged in #698.

Loïc Touraine and others added 5 commits December 9, 2021 10:15

feat[CLIUtils#682]: add the ability to set a config root folder. Usin…

9c9811c

…g set_config will search for the config filename from the current run directory of the application, when the file is not found in this situation, it is searched for from the config_root_folder when it is set.

style: pre-commit.ci fixes

786581c

fix: put back original set_config method prototype for backward compa…

5d825cd

…tibility.

Merge branch 'main' of https://github.com/Firefly35/CLI11

f14e00a

Fix: method ambiguity

cb113f8

phlptp reviewed Dec 13, 2021

View reviewed changes

phlptp closed this Jan 31, 2022

Firefly35 deleted the config_root_folder branch February 1, 2022 08:03

henryiii mentioned this pull request Feb 9, 2022

Add a transform/validator that checks for files on a default path. #698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add the ability to set a config root folder in set_config #684

Add the ability to set a config root folder in set_config #684

Uh oh!

Firefly35 commented Dec 9, 2021 •

edited

Loading

Uh oh!

phlptp commented Dec 13, 2021

Uh oh!

Firefly35 commented Dec 13, 2021 via email

Uh oh!

phlptp commented Dec 13, 2021

Uh oh!

Firefly35 commented Dec 13, 2021 via email

Uh oh!

phlptp Dec 13, 2021

Uh oh!

phlptp commented Dec 13, 2021 •

edited

Loading

Uh oh!

henryiii commented Jan 28, 2022

Uh oh!

Firefly35 commented Jan 28, 2022 •

edited

Loading

Uh oh!

henryiii commented Jan 28, 2022

Uh oh!

phlptp commented Jan 28, 2022

Uh oh!

Firefly35 commented Jan 28, 2022 via email

Uh oh!

phlptp commented Jan 29, 2022

Uh oh!

henryiii commented Jan 30, 2022

Uh oh!

phlptp commented Jan 31, 2022

Uh oh!

Uh oh!

Add the ability to set a config root folder in set_config #684

Add the ability to set a config root folder in set_config #684

Uh oh!

Conversation

Firefly35 commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phlptp commented Dec 13, 2021

Uh oh!

Firefly35 commented Dec 13, 2021 via email

Uh oh!

phlptp commented Dec 13, 2021

Uh oh!

Firefly35 commented Dec 13, 2021 via email

Uh oh!

phlptp Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

phlptp commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jan 28, 2022

Uh oh!

Firefly35 commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jan 28, 2022

Uh oh!

phlptp commented Jan 28, 2022

Uh oh!

Firefly35 commented Jan 28, 2022 via email

Uh oh!

phlptp commented Jan 29, 2022

Uh oh!

henryiii commented Jan 30, 2022

Uh oh!

phlptp commented Jan 31, 2022

Uh oh!

Uh oh!

Firefly35 commented Dec 9, 2021 •

edited

Loading

phlptp commented Dec 13, 2021 •

edited

Loading

Firefly35 commented Jan 28, 2022 •

edited

Loading