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

Empty comments #603

Open
Dahaka934 opened this issue Dec 3, 2018 · 5 comments
Open

Empty comments #603

Dahaka934 opened this issue Dec 3, 2018 · 5 comments

Comments

@Dahaka934
Copy link

Dahaka934 commented Dec 3, 2018

Why do you use a final private static default comment?

final private static ConfigOrigin defaultValueOrigin = SimpleConfigOrigin
            .newSimple("hardcoded value");

Maybe you add possibility to set default comment in ConfigRenderOptions, for example?

Also, if I use ConfigValueFactory.fromAnyRef(obj, "") then generated empty comment with '#'.

#
value=1

Maybe in the case of empty lines do not need to generate a comment?

@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

Skipping empty descriptions seems OK. Could allow null to be passed to fromAnyRef maybe and then skip null descriptions instead, might be cleaner, I think treating empty string as unset often ends in tears.

Implementation-wise I guess SimpleConfigOrigin would need the notion of an unset description and then at render time it could be skipped. I probably wouldn't make the ConfigOrigin.description getter return null (the docs state that it never will), instead I might make it able to have null internally, and return "hardcoded value" if it's null, and have a separate getter the render method can use like descriptionIsDefault() or something, to see if really it was unset.

For changing the default, the additional API and docs needed in ConfigRenderOptions for that feels a bit heavy to me. I think it might be hard to write the docs in a way that makes much sense. ("The description to render in a comment above values which were created in code rather than parsed from a file" ? seems a bit obscure, I feel like next to no one would use it...)

Note that if the format of the output matters to you much, the right direction is probably to be using and enhancing the ConfigDocument API (see #280 for history, #300 for one of the larger next steps). The Config object is meant to represent the actual runtime configuration, not a parse tree, so it does not preserve formatting or anything like that. The render method is really intended for debug output, not saving files, and the comments are only there to help with that. So I don't want to go overboard adding ways to fine-tune the render output. But easy fixes that don't add much API, sure.

@Dahaka934
Copy link
Author

Thanks for the detailed answer.
Could you fix at least the empty comments # ?
So that we can control the generation of comments when generating config.

@havocp
Copy link
Collaborator

havocp commented Dec 3, 2018

I don't have immediate plans to make this PR but if someone did I would try allowing a null originDescription when creating a new origin here:

private static ConfigOrigin valueOrigin(String originDescription) {
if (originDescription == null)
return defaultValueOrigin;
else
return SimpleConfigOrigin.newSimple(originDescription);
}

So allow a null originDescription to be passed in.

And have tests for the null-passed-in-to-ConfigValueFactory.fromAnyRef case, for sure.

Then rename SimpleConfigOrigin.description field to descriptionOrNull

In here if descriptionOrNull is null, default description to "hardcoded value"

Add a method to ConfigOrigin descriptionSetwhich returns descriptionOrNull != null and then in the render code, if the description is not set, skip the comment.

Not sure that will work out but that' what I'd try.

@Dahaka934
Copy link
Author

Why not just skip comment rendering if description().isEmpty()? And no need to add additional features.

@Dahaka934
Copy link
Author

Dahaka934 commented Dec 4, 2018

There are two places with next code:

if (options.getOriginComments()) {
String[] lines = v.origin().description().split("\n");
for (String l : lines) {
indent(sb, indent + 1, options);
sb.append('#');
if (!l.isEmpty())
sb.append(' ');
sb.append(l);
sb.append("\n");
}
}

Maybe, replace it to something like this?

if (options.getOriginComments()) {
    String desc = v.origin().description();
    if (!desc.isEmpty()) {
        String[] lines = desc.split("\n");
        for (String l : lines) {
            indent(sb, indent + 1, options);
            sb.append('#');
            if (!l.isEmpty())
                sb.append(' ');
            sb.append(l);
            sb.append("\n");
        }
    }
}

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

No branches or pull requests

2 participants