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

Allow to hide_locale when using host_locales config #239

Closed
wants to merge 2 commits into from
Closed

Allow to hide_locale when using host_locales config #239

wants to merge 2 commits into from

Conversation

WaKeMaTTa
Copy link

@WaKeMaTTa WaKeMaTTa commented Jan 13, 2021

This is what I want to accomplish in this Pull Request:

HOST PATH Should be accessible?
testapp.es / ✔️
/mostrar ✔️
/es/mostrar
/show
/en/show
testapp.co.uk / ✔️
/show ✔️
/en/show
/mostrar
/es/mostrar

If you run my spec in master branch:

You will see that fails.

@tagliala
Copy link
Collaborator

Hi!

Thanks for this PR

Could you please fix the RuboCop offences and push force?

Please run specs using rake so it will also perform the linting

Could you also please clarify what you are trying to archive?

Locale is automatically hidden when using host_locales config

@coveralls
Copy link

coveralls commented Jan 13, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling b13041a on WaKeMaTTa:allow-to-hide-locale-with-host-locales-config into f154fb1 on enriclluelles:master.

@WaKeMaTTa
Copy link
Author

@tagliala I updated the description Pull Request and I added some specs.

@WaKeMaTTa WaKeMaTTa marked this pull request as ready for review January 13, 2021 11:24
@tagliala
Copy link
Collaborator

tagliala commented Jan 13, 2021

Hi @WaKeMaTTa

Ok, I know exactly this issue (#171, from 2017) and you can find it fixed and documented on the branch 10-dev

Your tests pass on 10-dev.

Now I'm not using route_translator anymore in production, so that has not been merged because I don't know the side effects for other usages, but I can definitely release a version 10.0

Relevant commit and issues:

I did not release this change because the only comment in #171 is from someone that was using all the languages on all the domains. But again, I would support the case that makes most sense of all, leaving the developer the ability to provide a custom implementation

@tagliala tagliala closed this Jan 13, 2021
@WaKeMaTTa
Copy link
Author

It works as intended. Thanks.

@tagliala
Copy link
Collaborator

Welcome. @WaKeMaTTa what do you think about this issue?

Should we release a new major with that change?

Should we preserve the ability of loading all languages on all hosts?

@WaKeMaTTa
Copy link
Author

@tagliala I think this issue of having duplicated routes deserves a new major version. Because having more routes than necessary make things slower and confusing.

I saw that config verify_host_path_consistency in the branch 10-dev was removed AND it is the default behavior. I think is a good decision, because doesn't make sense to be able to access pages of other languages from other hosts.

@tagliala
Copy link
Collaborator

I think is a good decision, because doesn't make sense to be able to access pages of other languages from other hosts.

This weekend I will check if I can preserve the ability to have all languages on a single host for some edge cases (multi-domain / multi-language websites where the domain just sets the default locale), or provide some documentation on how to do that

I also think the time has come and I should merge this change in the stable version of Route Translator

@tagliala
Copy link
Collaborator

As per #171 (comment), I will release 10.0 with consistency lambdas enabled by default but I will keep the half-working implementation of the engine

I just need some tweaks to the wiki and to the docs

@tagliala
Copy link
Collaborator

@tagliala
Copy link
Collaborator

@WaKeMaTTa released, please give a try

@WaKeMaTTa
Copy link
Author

@tagliala done, looks great! 🎉

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