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

[MNG-7763] Provide UTF-8 as a default value for project.build.sourceEncoding and project.reporting.outputEncoding #1085

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Apr 13, 2023

…ncoding and project.reporting.outputEncoding
@michael-o
Copy link
Member

The outcome is then there is no way to remove this and it will also apply to ALL projects.

@gnodet
Copy link
Contributor Author

gnodet commented Apr 13, 2023

The outcome is then there is no way to remove this and it will also apply to ALL projects.

Isn't that the goal ?
Also you can't remove the property, but you can override it, which is very often the case (else you actually have a warning).

@michael-o
Copy link
Member

The outcome is then there is no way to remove this and it will also apply to ALL projects.

Isn't that the goal ? Also you can't remove the property, but you can override it, which is very often the case (else you actually have a warning).

Maybe, but this should be known that due to Plexus we cannot null values. @kwin Can we know? I don't that this is a huge problem here, but people should still know whether it is possible or not. I haven't used anything else, but UTF-8 for the past 20 years.

@rmannibucau
Copy link
Contributor

while you can override it in your own project (which is the case AFAIK) it is fine and I don't see how null and not be deterministic can be better than forcing the value to the OS one so guess we are good like that without handling null in the code.

@kwin
Copy link
Member

kwin commented Apr 13, 2023

FTR: overwriting default values by empty string is not supported due to https://github.com/eclipse/sisu.plexus/issues/29

@ge0ffrey
Copy link

ge0ffrey commented Apr 13, 2023

For those that want to override it to the Maven 3 behaviour, maybe you can introduce a magic value "OS_DEFAULT" that gets translated to not setting the encoding deeper down the implementation?

+1 to default to UTF-8

@michael-o
Copy link
Member

For those that want to override it to the Maven 3 behaviour, maybe you can introduce a magic value "OS_DEFAULT" that gets translated to not setting the encoding deeper down the implementation?

+1 to default to UTF-8

I'd prefer system. This is what I have done in other Maven components where empty was not allowed.

@ge0ffrey
Copy link

Good point.

@ge0ffrey
Copy link

ge0ffrey commented Apr 13, 2023

For what's it worth, I ran an online poll around this topic a few days ago:

image

I might have biased it a bit in how I posed the question :)

@gnodet
Copy link
Contributor Author

gnodet commented Apr 13, 2023

For those that want to override it to the Maven 3 behaviour, maybe you can introduce a magic value "OS_DEFAULT" that gets translated to not setting the encoding deeper down the implementation?
+1 to default to UTF-8

I'd prefer system. This is what I have done in other Maven components where empty was not allowed.

I'd suggest that people that want the default encoding use ${file.encoding} so that we don't have to do any hack in maven.

@michael-o
Copy link
Member

For those that want to override it to the Maven 3 behaviour, maybe you can introduce a magic value "OS_DEFAULT" that gets translated to not setting the encoding deeper down the implementation?
+1 to default to UTF-8

I'd prefer system. This is what I have done in other Maven components where empty was not allowed.

I'd suggest that people that want the default encoding use ${file.encoding} so that we don't have to do any hack in maven.

You are totally right. Stupid me.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the correction from @gnodet , I am fine with that. We need to properly document what is the scope of impact: Reactor or deps as well.

@gnodet gnodet merged commit 9e51a8f into apache:master Apr 17, 2023
@gnodet gnodet added this to the 4.0.0-alpha-6 milestone May 17, 2023
@gnodet gnodet deleted the MNG-7763 branch November 18, 2023 21:40
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

Successfully merging this pull request may close these issues.

8 participants