Cosmetic changes to LocalizationTable class #425
Conversation
| { | ||
| // The loop is searching for a value. Add the current char, regardless of what it is. | ||
| currentValue += c; | ||
| throw new InvalidOperationException(string.Format("Invalid format of localization string. Actual {0}", line)); |
There was a problem hiding this comment.
Are you sure about throwing exceptions here? For the record, I prefer crashing rather than having bad data. But in the rest of the code is seems more common to log error and then put an empty string there or something. You should maybe make sure that whoever calls initialize is prepared to catch the exception.
There was a problem hiding this comment.
At the very least make sure to log it
There was a problem hiding this comment.
@Mizipzor
At work I have practice - crash early. This could happen if someone provided bad date in localization file. This should be spotted.
There was a problem hiding this comment.
Yes, yes, yes I couldnt agree more. Crash early, crash often, I say. Maybe this should be discussed and settle on a standard. I have a feeling we are the minority. :P
There was a problem hiding this comment.
Crash early, crash often. Only if needed though. Doesn't make sense to crash on the user machine. You want to crash on dev machines
There was a problem hiding this comment.
@OzieGamma This is release or development still? How do you prefer to catch that someone screwed up with localization files?
There was a problem hiding this comment.
In a testing environment. Not sure about this particular case, my comment was general
|
@Mizipzor @OzieGamma Changed exception trowing with logging. Added line in the end |
|
Awesome. I would like to take this opportunity to highlight another cool Github feature. Do you see the discussions we had before, now that the lines have changes, that they are collapsed, since they are out of date? :) |
|
@Mizipzor Yes, interesting feature. |
|
@Mizipzor If I installed StyleCop to my Visual Studio where should I get configurations for in from? |
|
Theres the StyleCop wiki, but in short:
|
|
@Mizipzor So I do not need to configure it? It is already done on project level and my instance know what to do? |
|
You dont need to configure it, thats the point. :) There should be a |
|
Isn't it nice that you don't have to discuss this? You get a list of stuff to do, without room for interpretation, and you do it. And no one needs to be nagging. :) |
|
Merged! |
* Cosmetic changes to LocalizationTable class * Cosmetic changes to LocalizationTable class * Changed exception trowing with logging. Added line in the end * Fixed most of StyleCop warnings
* Cosmetic changes to LocalizationTable class * Cosmetic changes to LocalizationTable class * Changed exception trowing with logging. Added line in the end * Fixed most of StyleCop warnings
#375 Second try after all changes merged