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

Cosmetic changes to LocalizationTable class #375

Closed
wants to merge 3 commits into from
Closed

Cosmetic changes to LocalizationTable class #375

wants to merge 3 commits into from

Conversation

Grenkin1988
Copy link
Contributor

What do you think about this changes?
line.Split('='); i think .NET know how to work with strings efficiently?

@alexanderfast
Copy link
Collaborator

alexanderfast commented Aug 20, 2016

You seem to have done the same cleanup I did in #362. :)

Maybe base your work on that? Or should we just see whoever gets merged first? XD

Edit: But, yes, Split should absolutely be used here.

@Grenkin1988
Copy link
Contributor Author

Grenkin1988 commented Aug 20, 2016

@Mizipzor Yes, you've done more in missing localization part, and I changed other parts
I do not know how to get your chages first and than do my :-(

@alexanderfast
Copy link
Collaborator

I can probably merge your branch if you want. Or just replicate your changes, but then it would like look I committed them and its important to me that you get proper credit.

@RodRitter
Copy link
Collaborator

RodRitter commented Aug 20, 2016

I've just merged the cleanup from #362 - If you can pull and merge those changes in your code and solve the conflict, I can merge this. Then you both get your credit :)

@alexanderfast
Copy link
Collaborator

I just pulled his branch, there are merge conflicts but they are big and easy to see.

@Grenkin1988 Are you comfortable resolving merge conflicts? If not, this is an awesome learning experience. :) If you dont want to resolve but still want these changes in just toss your branch and start over based on latest master. Either way, I'm willing to help you with the Git wrangling, if you want.

@Grenkin1988
Copy link
Contributor Author

Grenkin1988 commented Aug 20, 2016

@Mizipzor @RodRitter Brobably this is it.

@alexanderfast
Copy link
Collaborator

Hm not really, you reverted most of my changes. :/

Consider installing and running StyleCop, thats what I did to guide my cleanup (StyleCop is the Microsoft style).

@Grenkin1988
Copy link
Contributor Author

@Mizipzor Than I will start again later in the evening. Need to go.

@alexanderfast
Copy link
Collaborator

Very well, take care. :)

@UnknownDude1
Copy link
Contributor

@Grenkin1988 By looking at your code for replacing my code with string.Split(), it seems that you implemented an exception if there are two or more '=' in a line. What I did was intentional, in case in any language for whatever reason an '=' is required for a localization that it still works.

@alexanderfast
Copy link
Collaborator

Its true that if the value itself contains a = then only the text between the first and second would be used, the rest would be skipped. This could easily be fixed by adding a max: .Split("=", 2);

@UnknownDude1
Copy link
Contributor

Alright, that I didn't know actually.

@Grenkin1988
Copy link
Contributor Author

@Mizipzor Could I close this PR and open new one? I am going to do total undo. Or it is neened for history tracking? (Never worked with git, I am using TFS at work)

@alexanderfast
Copy link
Collaborator

@Grenkin1988 If you want to start over that is absolutely what you should do. :)

@Grenkin1988
Copy link
Contributor Author

Closing this as going to undo everything and start from beginning

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.

4 participants