Skip to content
This repository has been archived by the owner on Nov 29, 2018. It is now read-only.

Fix issue #70 #113

Closed
wants to merge 2 commits into from
Closed

Fix issue #70 #113

wants to merge 2 commits into from

Conversation

hishamco
Copy link
Contributor

I made this PR to fix issue #70 , I added the a unit tests to cover the following cases:

  • If the request is not supported and DefaultRequestCulture is not defined
  • If the request is not supported and DefaultRequestCulture is not supported
  • If the request is not supported and SupportedCultures & SupportedUICultures are not defined
  • If the request is not supported and SupportedCultures & SupportedUICultures are empty

/cc @kirthik @Eilon

@hishamco
Copy link
Contributor Author

@kirthik can you please review this?

@kirthik
Copy link
Contributor

kirthik commented Oct 16, 2015

With this design change - #111, passing in default culture will be required when adding localization. We will scan request's culture through supported culture list and fall back to default if exact match is not found. So scenario where default culture is not defined or supported does not arise. I am not sure if this PR is relevant anymore.

@hishamco
Copy link
Contributor Author

I don't know why we should enforce the default culture while it's en-US in previous releases of ASP.NET?
If the PR irrelevant is that mean we should close issue #70 now?!!

@Eilon
Copy link
Member

Eilon commented Oct 18, 2015

#70 is probably not entirely needed because one of the reasons for the issue is that the default culture is optional. Once it's non-optional, most (or all?) of the issue goes away.

@kirthik
Copy link
Contributor

kirthik commented Oct 18, 2015

@Eilon: that's right. We can close this PR and #70

@Eilon
Copy link
Member

Eilon commented Oct 18, 2015

@kirthik do as you wish 😄

@kirthik kirthik closed this Oct 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants