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

Add EntityFramework Localization Sample #76

Closed
wants to merge 2 commits into from
Closed

Conversation

@hishamco
Copy link
Contributor

hishamco commented Nov 24, 2015

I see many users asking about localization sources, and how to use other sources such as json, database .. etc instead of resx files. I provide this example to use EntityFramework as source
/cc @davidfowl

@dnfclas
Copy link

dnfclas commented Nov 24, 2015

Hi @hishamco, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@hishamco
Copy link
Contributor Author

hishamco commented Nov 27, 2015

@Eilon @kirthik can you have a look if you don't mind, because I have seen many devs looking for such PR

@Eilon
Copy link
Member

Eilon commented Nov 28, 2015

Why open the same PR in two repos? This is a dup of aspnet/Localization#149, which is still being evaluated.

@Eilon Eilon closed this Nov 28, 2015
@hishamco
Copy link
Contributor Author

hishamco commented Nov 29, 2015

I'd like to let you know that I made the PR here after @davidfowl suggestion, and I forget to close the one in the localization repo, so if you think that the localiztion repo is the correct place that's fine

@davidfowl davidfowl reopened this Nov 29, 2015
@dnfclas
Copy link

dnfclas commented Nov 29, 2015

Hi @hishamco, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@davidfowl
Copy link
Member

davidfowl commented Nov 29, 2015

@Eilon we should close the other one.

@hishamco
Copy link
Contributor Author

hishamco commented Nov 29, 2015

I closed the other PR


public IEnumerable<LocalizedString> GetAllStrings(bool includeAncestorCultures)
{
return _db.Resources.Include(r => r.Culture).Where(r => r.Culture.Name == CultureInfo.CurrentCulture.Name)

This comment has been minimized.

@Eilon

Eilon Nov 30, 2015 Member

Recommend starting a new line on every new LINQ statement:

return _db.Resources
    .Include(r => r.Culture)
    .Where(r => r.Culture.Name == CultureInfo.CurrentCulture.Name)
    .Select(r => new LocalizedString(r.Key, r.Value, true)); // and use a named param for the bool

Please do this throughout the code in this PR.


using System.Collections.Generic;

namespace EFLocalizationSample.Model

This comment has been minimized.

@Eilon

Eilon Nov 30, 2015 Member

The namespace should also be .Models (fix in other files as well).

services.AddSingleton<IStringLocalizerFactory, EFStringLocalizerFactory>();
}

public void Configure(IApplicationBuilder app, IStringLocalizerFactory factory)

This comment has been minimized.

@Eilon

Eilon Nov 30, 2015 Member

Change param from factory to something more specific, e.g. localizerFactory.

@hishamco
Copy link
Contributor Author

hishamco commented Nov 30, 2015

The PR is updated
thanks @Eilon

@Eilon
Copy link
Member

Eilon commented Nov 30, 2015

@hishamco thanks for the update.

@DamianEdwards care to take a look and review?

@hishamco
Copy link
Contributor Author

hishamco commented Dec 7, 2015

Any updates regarding this PR?

@hishamco
Copy link
Contributor Author

hishamco commented Dec 8, 2015

@DamianEdwards is this looks good for you?

@Eilon
Copy link
Member

Eilon commented Dec 10, 2015

@hishamco we're working on some improvements to localization (@DamianEdwards is going to log some bugs soon) so we'll hold off on this PR until those changes go through. There will need to be some small changes to this PR but I don't think any major changes.

@hishamco
Copy link
Contributor Author

hishamco commented Dec 10, 2015

Thanks for clarifications @Eilon

@hishamco hishamco force-pushed the hishamco:dev branch from 8a913c9 to da90ee0 Dec 19, 2015
@hishamco
Copy link
Contributor Author

hishamco commented Dec 23, 2015

The PR has been updated to react to latest changes of localization & hosting APIs

@hishamco
Copy link
Contributor Author

hishamco commented Dec 23, 2015

@DamianEdwards is this PR fine along side with your last PR #83

@hishamco
Copy link
Contributor Author

hishamco commented Dec 28, 2015

@Eilon any updates regarding this

@Eilon
Copy link
Member

Eilon commented Jan 2, 2016

@hishamco sorry I was on vacation and was then sick.

Can you please squash the commits into one?

I think aside from that this looks :shipit: and I'll let @DamianEdwards merge it in if he doesn't spot anything else.

@hishamco
Copy link
Contributor Author

hishamco commented Jan 3, 2016

No problem @Eilon I hope you are fine now .. I will squash the commits

@hishamco hishamco force-pushed the hishamco:dev branch from 4e9d61b to c5634e9 Jan 3, 2016
@hishamco hishamco force-pushed the hishamco:dev branch from 599c0a3 to a4309a9 Jan 29, 2016
@hishamco
Copy link
Contributor Author

hishamco commented Feb 2, 2016

@Eilon any updates regarding this PR, because i'm planning to push another localization PR

public static void Main(string[] args)
{
var application = new WebApplicationBuilder()
.UseConfiguration(WebApplicationConfiguration.GetDefault(args))

This comment has been minimized.

@Eilon

Eilon Feb 2, 2016 Member

Could you add a call to .UseServer("Microsoft.AspNetCore.Server.Kestrel") here (e.g. see https://github.com/aspnet/Hosting/blob/dev/samples/SampleStartups/StartupFullControl.cs#L17) and delete hosting.json?

This comment has been minimized.

@hishamco

hishamco Feb 2, 2016 Author Contributor

Sure

}
},

"publishExclude": [

This comment has been minimized.

@Eilon

Eilon Feb 2, 2016 Member

Consider removing all these unneeded settings.

This comment has been minimized.

@hishamco

hishamco Feb 2, 2016 Author Contributor

Do you mean "compilationOptions" and "frameworks" sections?

This comment has been minimized.

@Eilon

Eilon Feb 2, 2016 Member

Just the unneeded publishExclude/exlude items. This project doesn't use node, etc. so there's nothing there to exclude.

React to new options pattern and hosting API changes

Add UseServer

Remove unneeded settings
@hishamco hishamco force-pushed the hishamco:dev branch from 9bca0bb to eaf1faa Feb 2, 2016
@hishamco
Copy link
Contributor Author

hishamco commented Feb 2, 2016

The PR has been updated and the commits squashed into single one

@Eilon
Copy link
Member

Eilon commented Feb 2, 2016

@hishamco thanks!

@ryanbrandenburg another one.

@ryanbrandenburg
Copy link
Member

ryanbrandenburg commented Feb 3, 2016

Shouldn't this be under the sample folder?

@hishamco
Copy link
Contributor Author

hishamco commented Feb 3, 2016

Yep, it should be under sample folder .. but I notice few days back it's outside the samples folder, I will push the change right now

@hishamco
Copy link
Contributor Author

hishamco commented Feb 3, 2016

The PR has been updated
thanks @ryanbrandenburg

@ryanbrandenburg
Copy link
Member

ryanbrandenburg commented Feb 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.