New var name in nc_rename_var() is not normalized for utf8. Should it be?

[edhartnett]

In nc_rename_dim() the new name is run through nc_utf8_normalize().

But this doesn't happen with nc_rename_var().

Shouldn't it?

[DennisHeimbigner]

yes, it should be normalized.

…

Sent from my iPad

On Dec 26, 2017, at 5:46 AM, Ed Hartnett ***@***.***> wrote: In nc_rename_dim() the new name is run through nc_utf8_normalize(). But this doesn't happen with nc_rename_var(). Shouldn't it? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[edhartnett]

What kind of string can I pass to make sure that normalization is taking place?

That is, what string can I pass that comes out differently after being normalized?

I would like to write some tests before fixing...

[DennisHeimbigner]

There should be a utf8 test case that tests normalization.

…

Sent from my iPad

On Dec 26, 2017, at 2:50 PM, Ed Hartnett ***@***.***> wrote: What kind of string can I pass to make sure that normalization is taking place? That is, what string can I pass that comes out differently after being normalized? I would like to write some tests before fixing... — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[edhartnett]

OK, I will hunt one down and add a test.

[DennisHeimbigner]

not obvious we need a rename normalization test specifically sine i presume you will use same
name check code used in e.g. nc_def_var

[edhartnett]

If we had a rename normalization test then this bug would have been noticed before. So of course we need a test for it.

[DennisHeimbigner]

I am confused. Did normalization fail or was it just missing?

[edhartnett]

Normalization is actually happening as I've just confirmed with a test. I will add the test to my current PR. When it's merged this ticket will be closed.