tag.Add: allows users to register custom or missing tags into the tags registry

[flicaflow]

From the discussion of the unmerged and probably obsolete PR #271 the idea was to Just a RegisterCustom function to the tag package. This PR is exactly that. I needed the TrackingID and TrackingUID tags which are currently not available. Following code enables me to use them now:

var (
	TagTrackingID  = tag.Tag{Group: 0x0062, Element: 0x0020}
	TagTrackingUID = tag.Tag{Group: 0x0062, Element: 0x0021}
)

func init() {
	tag.RegisterCustom(tag.Info{
		Tag:  TagTrackingID,
		VR:   "UT",
		Name: "TrackingID",
		VM:   "1",
	})
	tag.RegisterCustom(tag.Info{
		Tag:  TagTrackingUID,
		VR:   "UI",
		Name: "TrackingUID",
		VM:   "1",
	})
}

[suyashkumar]

Thank you @flicaflow! Couple thoughts:

• Can you add a couple sanity tests along with this? e.g. register a new tag and then check that you can successfully find it via one of the tag package APIs? (maybe in tag_test.go).
• How should we handle cases where a tag a user is trying to add already exists in the underlying dict? Should we return an error unless the user also sets an overrideExisting = true or something?

Otherwise, having a feature like this sounds good to me! :)

[flicaflow]

Hi @suyashkumar ,

• I agree, this code needs a unit test. I will add that.
• I as a user of the API would expect that Registering a tag will just do that and eventually overriding an existing one. Maybe that is even desired because you need to give it another VR type. Having this not as the default is dangerous because maybe with an update the list of pre-defined tags gets extended and suddenly my code breaks. Users can easily add a check to see if a tag already exists if desired. If we want to have this feature in it I would vote for a second function like RegisterCustomIfNotExist(...)

Thanks for you reply, the unit test follows soon.

[suyashkumar]

Thanks for the updates!

I as a user of the API would expect that Registering a tag will just do that and eventually overriding an existing one. Maybe that is even desired because you need to give it another VR type. Having this not as the default is dangerous because maybe with an update the list of pre-defined tags gets extended and suddenly my code breaks. Users can easily add a check to see if a tag already exists if desired. If we want to have this feature in it I would vote for a second function like RegisterCustomIfNotExist(...)

Interesting, I was somewhat worried about someone unknowingly updating a tag value that might be used or important in other places. This could also silently break or change logic the user may not expect or know how to debug. Plus, I suppose updating the library can always come with new errors returned that may not have previously, so what you say is always a possibility. I tend to want to err on the side of extra safety, particularly since tags are not updated often (and plus, it'd notify you that you could maybe remove your custom code), but I see your point. I wonder if we should rename the function. RegisterCustom makes it seem like it's for adding new "custom" tags vs. overriding existing ground truth data. WDYT about tag.SetInfo(tag.Info{})? The use of the Set verb makes it more implicitly clear that existing data may be changing, and of course we can continue to make that clear in the description. We can also include a note that you can check for existing tags before calling SetInfo if you want to ensure existing tags are not updated.

If we're worried about the override behavior, we could also throw a force boolean parameter into the function so the user has to be more explicit about this. Do you have a few other examples of when you'd be overriding existing tag info intentionally?

Other than that, overall lgtm with the minor changes suggested below.

WDYT?

[suyashkumar]

nit: you should be able to use cmp.Diff to check the info exactly matches the struct you created above: https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff

You could optionally check that both FindByName and Find returns the correct struct.

[suyashkumar]

nit: prefer t.Errorf here and above to collect all test errors. Typically I use t.Errorf for test failure conditions, and Fatalf for test setup issues where the test needs to stop immediately.

[flicaflow]

I absolutely agree with naming it SetInfo. I used RegisterCustom because you suggested it in the other PR long time ago. I also think the force flag is a good way to communicate what is happening.
In that case We could also just call it Set or even Add. This makes sense int the context of the tag package (tag.Set(...)) . Maybe Add does convey more the normal behavior (adding new tags) and the force flag makes it clear what is happening on collisions.

err := tag.Add(newTag, false) // errors if the tag already exists

err = tag.Add(newTag, true) // overwrites an eventually existing tag and never returns an error

[suyashkumar]

Makes sense, and I think that sounds good to me. tag.Add(info tag.Info, force bool) seems reasonable to me! It's a bit extra API than I'd usually prefer by adding the force flag, but I think in our case it can be good to force the user to decide and think about if they'd always want to override a colliding tag or not. Probably not a big deal either way, so happy to move forward with that! Thank you! :)

[flicaflow]

I completed the changes. Let me know what you think

[suyashkumar]

Looks good to me, thank you for the contribution!!! Just had a bunch of small style/opinion nitpicks that I added suggestions for (nothing major).

[flicaflow]

Thank you, I applied stylistic changes. I was thinking about defining an Error and didn't do it because I didn't see other examples in the package.
Cheers

[suyashkumar]

LGTM, with one minor typo. I'm not sure that I can commit to this branch otherwise I'd have sent the fix myself!

[suyashkumar]

Oh, jk, looks like it allowed me to add a commit!

[suyashkumar]

Merged 🎉 ! Thank you for the contribution @flicaflow :).