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

Resolve #43 #44

Merged
merged 8 commits into from
Apr 3, 2020
Merged

Resolve #43 #44

merged 8 commits into from
Apr 3, 2020

Conversation

shahabganji
Copy link
Contributor

When building the configuration an absolute path should be provided.

Copy link
Owner

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

If you can create a functional test for LocalizationSample.Mvc I will merge this soon, otherwise see my comments and I will add the functional test after

src/My.Extensions.Localization.Json/JsonStringLocalizer.cs Outdated Show resolved Hide resolved
samples/LocalizationSample/Controllers/TestController.cs Outdated Show resolved Hide resolved
samples/LocalizationSample/Startup.cs Outdated Show resolved Hide resolved
@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

I will add some commits to yours if you don't mind

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

@shahabganji are you sure both of functional tests are passed?

@shahabganji
Copy link
Contributor Author

@hishamco , yes pretty much sure

Screen Shot 2020-04-03 at 17 23 54

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Strange!! It doesn't work with me ;)

FunctionalTest

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Let me check what's prevent the test with French culture ...

@shahabganji
Copy link
Contributor Author

Would you try dotnet test via the command line?

Screen Shot 2020-04-03 at 17 33 11

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Sure, but it should be similar ;)

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Same thing, let me try something, coz the issue in the application root path

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Finally it got work!! I will update the PR soon, @shahabganji please check it again in your end

@shahabganji
Copy link
Contributor Author

shahabganji commented Apr 3, 2020

ping me whenever you've updated the PR, I will check it too. What was the cause of the problem ?

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

@shahabganji as I said earlier the issue comes from the application root path, please pull and test your functional tests with my latest changes

Also revert unnecessary changes in the samples files

@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

There 're two failing tests I need to fix after this PR, please run your added functional tests and revert the unnecessary changes

I will merge this when you confirm

Copy link
Owner

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting for your confirm if the functional tests are passed

@shahabganji
Copy link
Contributor Author

LGTM, all passed. well done 👍

@hishamco hishamco merged commit b14339e into hishamco:master Apr 3, 2020
@hishamco
Copy link
Owner

hishamco commented Apr 3, 2020

Thank you very much Saeed

@shahabganji
Copy link
Contributor Author

shahabganji commented Apr 3, 2020

My pleasure Hisham 👍 Please push a nuget package 🙏

@shahabganji shahabganji deleted the hotfix/absolutepath branch April 3, 2020 15:34
@hishamco
Copy link
Owner

hishamco commented Apr 7, 2020

FYI @shahabganji https://github.com/hishamco/My.Extensions.Localization.Json/releases/tag/v2.1, the NuGet package will be show up soon https://www.nuget.org/packages/My.Extensions.Localization.Json/2.1.0

@shahabganji
Copy link
Contributor Author

shahabganji commented Apr 8, 2020 via email

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.

None yet

2 participants