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

Feature Request: Template for dotenv configuration format #20

Open
cyphersnake opened this issue Oct 23, 2022 · 7 comments
Open

Feature Request: Template for dotenv configuration format #20

cyphersnake opened this issue Oct 23, 2022 · 7 comments

Comments

@cyphersnake
Copy link

I constantly need to give the .env.example file to the devops team and maintain it consistency with in-code structure by hand is inconvenient.

In the current implementation of Confique, the best solution is to give a template from a different format (like toml), where all the env keys are presented, but this does not look like the best solution. If there are no fundamental objections within the project, I will add a dotenv template by myself.

@LukasKalbertodt
Copy link
Owner

The problem is that confique deserialization from environment variables only works for basic types, but not lists or maps (also see #10). So I don't think .env is a format that can be fully supported. But of course not every configuration needs lists or maps, so I can see how it would be useful.

Note that you as a library user can generate the template for dotenv yourself by inspecting the Config::META information. That's what the existing template functions do. But yes, having this built-in might be nice. But again: what would the dotenv::template do if the config contains an array field for example? Skip it? Return an error? Still emit it into the template with a comment "you can't actually specify this via environment variable"? Do you have a good idea to solve this?

@cyphersnake
Copy link
Author

cyphersnake commented Oct 23, 2022

If it is impossible to present some configuration only through dotenv, then it would be good to know about it within the compile time. It seems to me, the template for dotenv simply shouldn't compile in that case (with an understandable and good error).

This, among other things, will avoid a potential error when a developer who is not familiar with this framework limitation cannot add such a array\map property.

@LukasKalbertodt
Copy link
Owner

The template functions cannot emit compile errors as they are simple functions running at runtime. And the Config derive macro doesn't know whether you just want to load it from env vars.

Finally, the derive macro can't know for sure whether a type can or cannot be deserialized well from an env variable. In case you're not familiar with proc macros: the macro only knows that you wrote Vec<u32> -- literally. It can't know whether this actually refers to std::vec::Vec or some other symbol. Sure, for Vec it's prooobably fine to assume it's always the one from std, but what if you use your own type PortList? Then the macro absolutely doesn't know whether it can be deserialized well from an env variable. So I don't think there is a good way to emit a compile error in those cases unfortunately.

@cyphersnake
Copy link
Author

cyphersnake commented Oct 24, 2022

Yes, I forgot about support for any deserializable type for other configuration providers.

In this case, since templates are generated at runtime, I think that it is more flexible to write warning in dotenv example. However, this task must wait for #10 to be implemented in the best possible way.

@cyphersnake
Copy link
Author

cyphersnake commented Nov 6, 2022

Now that #10 is closed, progress on this issue is possible

In general, the situation has not changed much, except that the env + parse_env attribute combination no longer needs a warning for complex types.

However, since support is implemented through a simple function, then annotations with a possible format will have to be added manually (in rustdoc) if some auxiliary function built into the library is not used. If the list_by_* function is used, then add a message about it to template.

Summary:

  • Warning for complex types, without parse_env attr
  • Format and delimiter message if one of the built-in helper methods is used
  • A message that the field is supported, without specifying the format, leaving this opportunity to the author of the configuration structure

Feel free to correct my judgment @LukasKalbertodt

@LukasKalbertodt
Copy link
Owner

LukasKalbertodt commented Nov 6, 2022

If the list_by_* function is used, then add a message about it to template.

We can't know that. Right now, meta types do not contain information about parse_env. But even if we wanted to add that... how? We can't really resolve paths in the proc macro, so we can only guess from the literal path. And I think I would rather not do that as it's too brittle.

Warning for complex types, without parse_env attr

Similar thing here. First: how to warn? template functions shouldn't just print something to stdout/stderr. One could write a warning to the output string, but that's just hoping the developer would notice. But also, we can't judge whether something is a complex type or not.

So what I'm basically saying: we can't check in env::template whether the field can even be loaded properly from an environment variable. So that's just the responsibility of the library user then.


Now. What about default values of type array or map? That's a tricky question. As I said above, we cannot gain any useful information from the parse_env attribute, unfortunately. Yes, for the built-in list_by* it likely works most of the time, but in general we can't.

So I think I see a number of options:

  • (a) Make env::template not output the default value for arrays or maps at all. Let the user describe the default in the comment manually.
  • (b) Add a special default_env = "8080,8090" attribute that is only used for env::template.
  • (c) Add two functions to env::FormatOptions, e.g. format_array and format_map, which are used by env::template to convert the array/map into a string that can be used as default env value. But that assumes the array syntax is the same for all config fields... so maybe pass the path of the config field to format_array? Meh...
  • (d) Add a general system of specifying format specific things, e.g. #[config(default = [8080, 8090], env.default = "8080,8090")]. I noticed a number of times that people might want to do that for various reasons, also for other formats.

One other idea is to let people specify default = "8080,8090", i.e. specify the default not as the proper type (array) but rather as the env string. That would be easy to use in env::template then. But the default value is deserialized without parse_env. But if the user wants to specify the default as env string, they very likely exclusively load values from env vars and not any other file. So the user can just use deserialize_with and specify the default as env var.

I'm not sure about any of this. But having thought it through and written down now, I think I would suggest (a). It's certainly the easiest. Its disadvantage of course is that people have to write the default value twice (in the default attribute and in the comment. But we can always expand on this and provide a better solution in the future. I dislike (b) and (c) for various reasons. (d) is probably the solution I would go with for the long term, but there is still too much design to be done there and I wouldn't want to implement it now.

So yeah: thoughts or opinions on this? Would you agree with my conclusion of going with (a) for now?

@cyphersnake
Copy link
Author

@LukasKalbertodt I agree that one can start with (a). At least there is no problem improve this solution in the future.

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

No branches or pull requests

2 participants