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

Accept default value in placeholders for unresolved property values #1

Merged
merged 5 commits into from May 23, 2023

Conversation

diamondT
Copy link
Contributor

Hi,

Here is a patch for accepting default values when a property key is not resolved.
Default value (if present) will be returned if all other resolution attempts fail (i.e. properties -> system properties -> env variable -> default value).

A few examples:

Given these initial values

("host", "example.com")
("port", "9090")
("fallback", "fallback value")

the following replacements will happen

raw value replacement
${unknown:} ${unknown}
${unknown:defaultValue} defaultValue
http://${uhost:localhost}:${uport:8080} http://localhost:8080
http://${host:localhost}:${port:8080} http://example.com:9090
${unknown:${fallback}} fallback value
${unknown:${double.unknown}} ${double.unknown}
${unknown:with space} with space
${unknown:with extra :} with extra :
${malformed:defVal ${malformed:defVal
${malformed:with space ${malformed:with space
${malformed:with extra : ${malformed:with extra :
${unknown::} :

Let me know what you think.

Thanks,

@khmarbaise
Copy link
Member

If i correctly understand your code this will handle all the time the default values...I would say to add a flag which controls this behaviour and don't make it the default. Furthermore it would be nice having a documentation on the site with an example...

@diamondT
Copy link
Contributor Author

Done.
I assume by "site" you meant the "index.apt" file. I've added a few lines there along with the examples mentioned in my previous comment.

Thanks,

@borisbrodski
Copy link
Contributor

This is a really cool feature! I vote for it.

Just one note: In my opinion, ${test:} should be considered as a property with empty default (default="").
So the expansion may look like this:

raw value replacement
${unknown} ${unknown}
${unknown:}

What do you think?

@vguna
Copy link

vguna commented Jul 7, 2017

Sounds nice. Any news on this?

ecararus pushed a commit to ecararus/properties-maven-plugin that referenced this pull request Jul 11, 2017
@OrangeDog
Copy link

Can this be revisited?

@oliverlockwood
Copy link

This would be incredibly useful. Perhaps one of the project maintainers could advise what's necessary to be able to merge this in and start using it? I'd rather not have to maintain a fork of this plugin for something which ought to be common usage!

@hinorashi
Copy link

hi guys, how about this, i'm still waiting for it :D

@eugenepaniot
Copy link

Me too, still waiting for it -(

@ProIcons
Copy link

ProIcons commented Nov 2, 2021

bump

@drekbour
Copy link

drekbour commented Jan 7, 2022

2022 and still waiting...

@diamondT
Copy link
Contributor Author

well after all this time there had to be a few conflicts :)
resolved.

@drekbour
Copy link

Hmmm. I actually wanted this behaviour for vanilla <properties> in the pom.xml. Does this plugin enable that in any way?

@diamondT
Copy link
Contributor Author

resolved some more conflicts with the base branch 🔀

@slawekjaranowski slawekjaranowski self-assigned this May 23, 2023
@slawekjaranowski slawekjaranowski merged commit 2a3ff69 into mojohaus:master May 23, 2023
@slawekjaranowski
Copy link
Member

@diamondT thanks for PR and for patience
This one is the oldest PR I was merged as I remember 😄

@slawekjaranowski slawekjaranowski added this to the next-release milestone May 23, 2023
@diamondT
Copy link
Contributor Author

better late than never :)
cheers!

mykelalvis added a commit to infrastructurebuilder/archived-properties-maven-plugin that referenced this pull request Sep 20, 2023
* Migrate changes to plugin from upstream.  Fixes mojohaus#1

This is the change set for the new fork.

* Add run-its to release process

* Add mojorelease to release process
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.

None yet