Add more coverage for UTF8String

[ScottPJones]

This should give almost 100% coverage of unicode/utf8.jl, there is still one line with reverse that looks like the C code u8_reverse is broken that I'll need to address in a subsequent PR.

[IainNZ]

Stupid questions maybe, but are these methods used anywhere in Base? If they are not, then we should get rid of them probably. If they are used, can we reach these lines through another higher level method? And if not, why not? Do they represent things that can't happen?

[ScottPJones]

No, but since there is currently no easy way of deprecating a unexported function (unless #11976 is merged), and there are packages using them, I can't get rid of them at the moment.

[ScottPJones]

(and not a stupid question either! I especially didn't want to break JuliaParse right yet, which depends on the utf8sizeof [which I don't think is even correct], because Coverage depends on it!)

[tkelman]

Since you know what packages are using them at least out of public registered code, would it be possible to pre-emptively move them to some replacement? Or would that just be copy-pasting the base code for the sake of being able to rename/delete code here?

[ScottPJones]

I can submit a PR to change JuliaParser tomorrow, I just discovered the dependency after I saw that the function was never used in base, but I want to check something where I think the current definition is incorrect first. The other one is in MutableStrings, using a function I'd already renamed and made generic.

[IainNZ]

Coverage no longer depends on JuliaParser, and JuliaParser is already broken, so I think its a good idea to just remove them if its dead code.
With a notification to @jakebolewski that he might want to put them somewhere if he needs them for the parser.
I don't think we need to worry about handling unexported code, thats just too much for anyone to waste time on.

[ScottPJones]

I was going to fork JuliaParser and submit a PR with the fix, but git says JuliaParser cannot be forked. @jakebolewski What's up?

[IainNZ]

I can fork it... You sure you haven't already forked it?

[ScottPJones]

I'm sure, you can look at my repositories.
I keep getting this message:
Cannot fork this repository

[tkelman]

You can try with your other account, if you still have that from when github thought you were a robot. Generally github discourages having multiple accounts though.

[ScottPJones]

Is there anything I should change in this PR right now, or could somebody merge it?
(I have another PR that to fix the bugs in reverse of UTF8String, that I also want to add testing for in this same file, and in a later PR I can make sure that the two unexported functions are removed, tests removed, and also submit PRs to fix MutableStrings and JuliaParser)

[IainNZ]

I don't it should be merged with those tests for unexported unused methods included, because ideally a new PR would remove. The rest is fine.

[ScottPJones]

The problem is, they are used (in registered packages). I thought it would be better to add the tests, to achieve 100% coverage (which was my goal), and then, after fixing the packages, remove the code and the tests.

[IainNZ]

Disagree, I don't care that registered packages won't work (like I said, JuliaParser doesn't work anyway last time I checked). We do not support unexported functions.

[ScottPJones]

@IainNZ it is not true that all unexported functions are not supported. Look at things like to_index and compilecache. Not exporting a name is useful to avoid name pollution, or to indicate that "here be dragons", which was the case with compilecache.
The other point, is that everything in base should have full testing coverage (just ask @kshyatt!), whether exported or not.
Just because to_index for example is not exported, it should not be ignored in unit testing.

[IainNZ]

These methods are not only not exported, but aren't used in Base. If they were used in Base, then they sure they should be covered. They aren't, they are unsupported, and they are going to be removed. Why add unit tests for something that is about to be removed? If you take the tests out I'll merge this right now. Feel free to add the tests in another PR (that probably won't be merged).

Or alternatively, convince someone else to merge this. Either way, I"m going to unsubscribe from this PR now and let someone else argue with you, because I think we are wasting each others time.

[tknopp]

I close this one but ping @StefanKarpinski and @tkelman if they are of a different opinion. @IainNZ made good points which were ignored by the OP.

[ScottPJones]

@tknopp Can you reopen this? I just got back home, and was about to push a commit with the tests that @IainNZ didn't like removed, so that the other tests (which are needed) could go in.

[ScottPJones]

@tknopp I did not ignore anything that @IainNZ said, I was away for most of the day, also he never responded about cases I raised such as to_index or compilecache. If either of you will reopen this, I can push a commit without those tests.

[ScottPJones]

@StefanKarpinski @tkelman I'd like to get the rest of the unit tests in (without the ones that @IainNZ didn't like) but since this has been closed on me, I can't push the commit. Could one of you please reopen this so I can do so?

[tkelman]

he never responded about cases I raised such as to_index or compilecache.

Yes, he did. The distinction there is used in base vs unused in base. It's more productive to fix the uses in packages ahead of time if we know where they are than it is to argue about it.

[ScottPJones]

OK, he responded making that distinction about unused in Base after I had left for the day.
I would have fixed the package instead, but as I said, for some reason I was not able to from my @ScottPJones account. Tonight I did try as you suggested, with my emergency account from my days as a robot, that worked, and is JuliaLang/JuliaParser.jl#21.
I have also removed the two functions in question from Base in another PR #12655

[ScottPJones]

Bump: everything has passed