search

PowerShell / DscResource.Template

MIT License
10 stars 15 forks source link

DscResource.LocalizationHelper Module is misnamed #21

Closed PlagueHO closed 5 years ago

PlagueHO commented 5 years ago

Having the Exception helper functions in the DscResource.LocalizationHelper module is misleading. These functions are not related to localization (and can be used without). I don't think users would expect to find exception functions in there.

I think this module should be renamed to something more generalized such as 'DscResource.ResourceHelper'.

This also relates to this issue: https://github.com/PowerShell/DscResources/issues/476

Another alternative would be to move the exception functions into DscResource.Common.

johlju commented 5 years ago

We could just move all the functions to DscResource.Common instead. Only draw back is that loading localization strings for the Common module needs to be done right after the Get-LocalizedData function is defined. The names DscResource.ResourceHelper and DscResource.Common is the same thing. How should contributors know what would go where? Better to just have one DscResource.Common?

PlagueHO commented 5 years ago

I did consider that option, but wasn't sure it was a good idea because of the problem you mention.

I too have the exact same thoughts as you - there seems to be two places to put generic shared functions and it doesn't make a whole lot of sense.

I'd much prefer to have a single 'DscResource.Common'.

Any other options? @gaelcolas ?

PlagueHO commented 5 years ago

Actually, just ran into an issue with the Common Tests - Validate Localization that relates to this. Because the DSCResource.LocalizationHelper itself does not have an en-US folder it actually fails these tests: https://ci.appveyor.com/project/PlagueHO/iscsidsc/builds/24298674#L529

I'm going to work around this in the resources I maintain by combining the all the common modules into the DscResource.Common module, but we probably need to come up with a solution to this over in DSCResources.Tests as well (I'll raise the issue over there too).

johlju commented 5 years ago

Because the DSCResource.LocalizationHelper itself does not have an en-US folder it actually fails these tests

Yes, a string resource file with no keys is the minimum that is required. I didn't want to override (filter out) that in the DscResource.Test. Until there was a better solution I thought the best workaround was to add a string resource file with no keys since all modules and resource should be localized.

we probably need to come up with a solution to this over in DSCResources.Tests as well

I think the solution we discuss here will see if we need to make changes to DscResource.Tests.

PlagueHO commented 5 years ago

That is what I did. But I actually think in the case of DscResource.Localization it is just better to merge it to Common (which is what I'm starting to do).

johlju commented 5 years ago

Sounds good. Let's do that.

PlagueHO commented 5 years ago

Ok. I'll submit a PR.