search

sillsdev / l10nsharp

A .NET localization library for Windows Forms applications.
3 stars 10 forks source link

Fix broken Group tests #3

Closed gmartin7 closed 8 years ago

gmartin7 commented 8 years ago

This change is Reviewable

jasonleenaylor commented 8 years ago

So I see how you made the test pass but I'm not sure I understand the impact for users of the library. So when a user decides a string is no longer used they tag it somehow?

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

src/L10NSharp/LocalizedStringCache.cs, line 613 at r1 (raw file):

                  {
                      newNode = new LocTreeNode(OwningManager, groupChain[i], null, nodeKey);
                      nodeCollection.Add(newNode);

Was this intended?

Comments from Reviewable

jasonleenaylor commented 8 years ago

Reviewed 1 of 2 files at r1. Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

jasonleenaylor commented 8 years ago

Reviewed 1 of 2 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

gmartin7 commented 8 years ago

Yes, they mark it with a nolongerused property. And anyway, this code is in the part that handles the x-group property which I've never seen used before.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

_Comments from Reviewable_

gmartin7 commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/L10NSharp/LocalizedStringCache.cs, line 613 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote… > Was this intended? >

Yes, I couldn't see any possible reason to add something to a collection that was being overwritten on the next line.

Comments from Reviewable

jasonleenaylor commented 8 years ago
:lgtm:

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

src/L10NSharp/LocalizedStringCache.cs, line 613 at r1 (raw file):

Previously, gmartin7 (Gordon Martin) wrote… > Yes, I couldn't see any possible reason to add something to a collection that was being overwritten on the next line. >

I suppose, but I'm curious about how it works with newNode. I see you didn't break anything so I don't really have any objections. :smile:

Comments from Reviewable

gmartin7 commented 8 years ago

I'll admit to a similar curiosity! :worried:

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable