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

default_test option #4

Closed
mgibowski opened this issue Mar 6, 2023 · 8 comments · Fixed by #5 or #6
Closed

default_test option #4

mgibowski opened this issue Mar 6, 2023 · 8 comments · Fixed by #5 or #6
Assignees
Labels
enhancement New feature or request

Comments

@mgibowski
Copy link

Hi, cool library :)

It would be great to have a default_test option, similar to default_dev.

@emadalam
Copy link
Owner

emadalam commented Mar 6, 2023

@mgibowski Thanks for trying it out and giving the feedback 🙌

Currently the default_dev works for both :dev and :test mix environments. The name might have been a little confusing but the idea was to have a default that can work for local development (that includes both :dev and :test).

Having said that, I'm still contemplating what could be the best way to allow setting mix environment specific defaults since mix environments can be any number of custom values. I'm thinking of introducing a unified defaults keyword list option to allow this flexibility. Any other suggestions are more than welcome.

@mgibowski
Copy link
Author

In my case, I just need a different default value for test, different for dev, and no default for prod (always set from env variable).

defaults: [dev: “a”, test: “b”]would be fine for my use case.

@emadalam
Copy link
Owner

emadalam commented Mar 7, 2023

@mgibowski I have added the configuration option defaults as part of a new feature branch. Have a look at this PR #5 and see if it serves the purpose. I'd make a release tomorrow if you don't have any further comments to the approach.

Side note: I have also soft deprecated the default_dev option with warnings to prefer using the new defaults option.

@emadalam emadalam added the enhancement New feature or request label Mar 7, 2023
@emadalam emadalam self-assigned this Mar 7, 2023
@mgibowski
Copy link
Author

thanks @emadalam just tried the feature branch, and it works great for me!

@mgibowski
Copy link
Author

@emadalam I was a bit too quick.

Something doesn't work.

Here is the code fragment:

  use Mahaul,
    REDPANDA_NUM_PARTITIONS: [type: :int, defaults: [dev: 1]]

When I invoke MyApp.Env.redpanda_num_partitions()

I get the error:

** (FunctionClauseError) no function clause matching in Integer.parse/2    
    
    The following arguments were given to Integer.parse/2:
    
        # 1
        1
    
        # 2
        10
    
    Attempted function clauses (showing 2 out of 2):
    
        def parse(_binary, base) when not (is_integer(base) and (base >= 2 and base <= 36))
        def parse(binary, base) when is_binary(binary)
    
    (elixir 1.14.3) lib/integer.ex:275: Integer.parse/2
    (mahaul 0.4.0) lib/mahaul/helpers.ex:226: Mahaul.Helpers.parse/2

Is it a bug in mahaul, or did I make some mistake?

@emadalam
Copy link
Owner

emadalam commented Mar 7, 2023

@mgibowski For consistency reasons, the parsing of values to correct data types happen only from string values like how you set in the actual environment variables. So the correct way to set any defaults is to set the string values exactly like how you would do while setting the environment variables. So in your example you should do,

  use Mahaul,
    REDPANDA_NUM_PARTITIONS: [
      type: :int,
      defaults: [dev: "1"]
    ]

Regardless of the type, you always should set string values for defaults, I have it in the documentation, but maybe I'd need to reemphasize that and improve documentation. Also I could improve this through a compile time validation in future.

@mgibowski
Copy link
Author

Alright, makes sense - thanks!

@emadalam
Copy link
Owner

emadalam commented Mar 7, 2023

Also I could improve this through a compile time validation in future.

@mgibowski In retrospect, this should have been implemented with the new defaults option, as we already had compile time checks for invalid default and default_dev options.

Nonetheless I have added those compile time checks in v0.4.1 🙌

Screenshot 2023-03-07 at 16 40 19

Screenshot 2023-03-07 at 16 39 58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants