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

FacesConfig#version() cannot return null #5210

Merged
merged 1 commit into from
Jan 31, 2023
Merged

FacesConfig#version() cannot return null #5210

merged 1 commit into from
Jan 31, 2023

Conversation

BalusC
Copy link
Contributor

@BalusC BalusC commented Jan 28, 2023

FacesConfig#version() cannot return null
@BalusC
Copy link
Contributor Author

BalusC commented Jan 28, 2023

@arjantijms and @tandraschko WDYT? Wondering between returning JSF_2_3 or adding something like NULL or LATEST as this isn't intended to be ever used anywhere and would be the first thing ever in Faces or perhaps even EE which is already deprecated at moment of addition 🙄

@tandraschko
Copy link
Contributor

then lets return JSF_2_3 for now, we will remove it in 4.1 anyway?

@BalusC
Copy link
Contributor Author

BalusC commented Jan 30, 2023

Right.

The only issue is that it's an API change and hence this PR targeted 4.1 instead of 4.0. But indeed we're going to remove it nonetheless. I'm only wondering what to do with 4.0. Leave it broken in this regard?

@tandraschko
Copy link
Contributor

Cant we just add it to 4.0?

@BalusC
Copy link
Contributor Author

BalusC commented Jan 30, 2023

I'm all for it because it's clearly a bug, but technically speaking, it's still an API change and these should go in a minor/major version.

@arjantijms can't we make an exception for this one?

@tandraschko
Copy link
Contributor

for me its ok and already commited to MF
as long as we dont change method signatures, its not a problem IMO

@BalusC BalusC changed the base branch from master to 4.0 January 30, 2023 13:20
@BalusC
Copy link
Contributor Author

BalusC commented Jan 30, 2023

Fair point. Ok, so be it.

@arjantijms
Copy link
Contributor

I gave this some extra thought, and I now think it's an implementation change/bug fix, and not an API one.

The body of the version() method is implementation. By definition, it should return whatever the main annotation has as a default value:

See

Version version() default Version.JSF_2_3;

@BalusC
Copy link
Contributor Author

BalusC commented Jan 30, 2023

Bon. Let's approve it then.

@arjantijms arjantijms merged commit 1375d38 into 4.0 Jan 31, 2023
@BalusC BalusC deleted the faces_issue_1784 branch February 6, 2023 11:04
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.

3 participants