Convert '-' to '_' in summary dataframe

[ns-rse]

No description provided.

[jni]

Ach — looks like your editor had string normalisation on, as well as a few other settings that caused unrelated changes (removing some parentheses, changing 1. to 1.0...). Do you think you could find your fix and try again without all the changes?

[ns-rse]

Sorry about that, I'd setup the local repository to apply yapf automatically on commits using pre-commit but its not quite working as I expected. Will make sure future PRs don't have that problem.

[ns-rse]

Anything else required for this @jni ?

[jni]

@ns-rse Sorry, I assumed you were still working on this! Have you looked at the changes tab? There's still many unrelated changes — many look like black, e.g. line length exceeding 80c, 4 space indents...

[ns-rse]

Sorry @jni I thought I'd caught everything, will sort things out now.

[ns-rse]

Applied yapf via pre-commit run --all-files (twice as first time I was rushing and didn't remove the # yapf: disable / # yapf: ignore lines).

[ns-rse]

Was scratching my head 🤔 as to why the tests were failing until I realised my fork/branch were behind.

Hopefully sorted now.

[jni]

@ns-rse cool, this is ready now. I was about to merge but then I got worried that it would be a very disruptive change for our users. I think we should add a keyword argument to deprecate the - over two releases. It would be relatively easy:

• add a keyword argument, say, separator=None;
• in the code, do:

if separator is None:
    separator = '-';
    warnings.warn(
            "separator in column name will change to _ in next version; "
            "use `separator='-'` to maintain current behaviour and silence this warning; "
            "use `separator='_'` to switch to the new behaviour and silence this warning."
            )

• at the end of the function, do summary.rename(columns=lambda s: s.replace('_', separator)
• in all the tests, call the function with separator='_'
• finally, add a test with with pytests.warns that calls it without a separator argument to check that the default works.

Actually I might just push the changes directly and then merge. 😂 I hope this solution is ok with you!

[jni]

Well I'm glad I did that cos it made me check the docs, which needed to be updated with the new separator. @ns-rse can you sanity-check for me that this is an ok approach? I was a bit annoyed with adding separator= everywhere, but at the same time, it's a lot easier for downstream users to do separator='-' to their summarize calls, rather than hunt down every downstream usage of a column identifier...

[ns-rse]

Sorry for missing updates to documentation.

I agree that its sensible to deprecate this with warnings and give people the opportunity to phase things out and this looks like a sensible approach. I've never undertaken deprecations before so I've learnt about when to use DeprecationWarning v FutureWarning. Do you think skan might be being used by other developers and warrant a DeprecationWarning?

One minor suggestion I have is to raise the warning when separator = '-' is explicitly given as whilst the chances are people will have read the warning at least once if they are explicitly specifying it, it would perhaps be useful to remind them of the need to change existing code.

[jni]

What you say is the correct approach if we plan to remove support for - in the future. However, I don't think we want to do that, because then users have to go through two deprecation cycles: one to set separator='_', then another to remove the separator argument. That's annoying, and maybe people would be happy with using separator='-' forever — we don't have to use it in our docs and/or internal usage. So, we can decide to warn only when separator is not given, and keep separator around ~forever, or until 1.0.

Probably I would do VisibleDeprecationWarning rather than DeprecationWarning, as many users don't see DeprecationWarnings. But I agree that it's better than FutureWarning.

[jni]

🤦 I completely forgot to update the docs, sorry.

Don't worry, I would have forgotten too if I hadn't git grep summarized. I need to add tests of the docs build. Actually there's other parts of the docs that are broken right now, but I left fixing them to another PR.

[ns-rse]

Actually there's other parts of the docs that are broken right now, but I left fixing them to another PR.

The pytest-doctestplus plugin might be of some use here.