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

Allow use of yamerl without starting the app #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

artman41
Copy link

Forces an application load prior to the application:get_env/2

Forces an application load prior to the `application:get_env/2`
@coveralls
Copy link

Coverage Status

coverage: 77.575% (+0.01%) from 77.564%
when pulling 4722d1e on artman41:patch-1
into a24f448 on yakaz:master.

@dumbbell
Copy link
Collaborator

Hi! Thank you for the contribution!

application:load/1 is a synchronous call to the application controller, thus it can be expensive.

I see two possible approaches:

  1. Load the application in e.g. yamerl_constr:setup_node/2 which semantically is supposed to be called once unlike yamerl_app:get_param/1. We can also check if the application is already loaded before with application:loaded_application/0 which is a simple ETS read.
  2. Be optimistic in yamerl_app:get_param/1 and call application:get_env/2 in a case and if it returns undefined, load the application and call application:get_env/2 again.

What do you think? I have a slight preference for option 1.

@dumbbell dumbbell self-assigned this Jan 23, 2025
@artman41
Copy link
Author

since the app resource defines the default env as {node_mods, []}, I'd be tempted to just swap the application:env_env/2 to an application:get_env/3 where it would default to an empty list

Otherwise, having a forced load occur in the setup would be a good middleground 👍

@dumbbell
Copy link
Collaborator

You are right this is a better idea. Would you mind modifying your pull request to use application:get_env/3?

@artman41
Copy link
Author

artman41 commented Jan 29, 2025

@dumbbell have a look and let me know what you think 👍

My go-to style is to have a config module that'd look something like this

-module(yamerl_config).

-export([
    get_node_mods/0,
    set_node_mods/1
]).

-spec get_node_mods() -> list(module()).
get_node_mods() ->
    get_value(node_mods, []).

-spec set_node_mods(list(module())) -> ok.
set_node_mods(Value) ->
    set_value(node_mods, Value).
    
%% Internal Functions

get_value(Key) ->
    application:get_env(yamerl, Key).
    
get_value(Key, Default) ->
    case get_value(Key) of
        undefined ->
            Default;
        {ok, Value} ->
            Value
    end. 
    
set_value(Key, Value) ->
    application:set_env(yamerl, Key, Value).

But I've tried to fit with the existing style 😄

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@dumbbell
Copy link
Collaborator

For very simple parameter handling like here, I usually put that in the *_app.erl module.

I like your suggestion to use dedicated functions like get_node_mods(). Don’t hesitate to apply that to this pull request if you like :)

Otherwise, please squash all commits into one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants