Hello and thanks for the PR!

Here I would recomment keeping only the example with the enum, since it’s the way that we want to recommend.

To match the import style (see import pstats in the previous code block), the example would need to be p.sort_stats(pstats.SortKey.NAME) (assuming the upper-case fix asked by the other reviewer is done).

I wonder if the enum should be defined on the Stats class, to make code shorter: p.sort_stats(p.SortKey.NAME). Maybe worth asking on the ticket to get other people’s opinion.

I would change the import to

from pstats import Stats, SortKey

The pstats module is all about stats processing, so there is no need to hide SortKey inside the Stats class.

Thank you for the comment. Re: 'Here I would recomment keeping only the example with the enum, since it’s the way that we want to recommend.' I could do that, but I have also received comment about having both example as the string argument is still valid. Showing only the enum would make the doc incomplete.

Yes there is a need to also show an example with a string form, but not all code examples need both forms.

@mwidjaja

When you make the duplicate SortKeys actually duplicates, please put the better name first -- something like:

class SortKey(str, Enum):
    calls = 'calls'
    ncalls = 'calls'
    cumulative = 'cumulative'
    cumtime = 'cumulative'
    module = 'module'
    file = 'module'
    filename = 'module'
    line = 'line'
    name = 'name'
    nfl = 'nfl'
    pcalls = 'pcalls'
    stdname = 'stdname'
    time = 'time'
    tottime = 'time'

I still think showing both ways in these examples is not useful and could create confusion.
It would be enough to have one example here and the changes that you already did in the method docs later in this file.

Ok. unless there're objections from anyone else, I'll remove them and just keep the one in the method doc.

If people follow the code examples here, SortKey will be undefined. It needs to be imported.

enum*

Since we still have to support string arguments, and the SortKey members are also strings, I would just leave the original code alone here.

My initial understanding of this was flawed. Since we need to support the old string values we can't just have CUMTIME and CUMULATIVE map to "cumulative" because then "cumtime" is gone (unless we do extra work, which I see you did).

What we need here is a MultiValueEnum, so SortKey.CUMULATIVE maps to both "cumulative" and "cumtime".

The Enum should look like this:

class SortKey(str, Enum):

    CALLS = 'calls', 'ncalls'
    CUMULATIVE = 'cumulative', "cumtime"
    MODULE = 'module', 'filename', 'file'
    LINE = 'line'
    NAME = 'name'
    NFL = 'nfl'
    PCALLS = 'pcalls'
    STDNAME = 'stdname'
    TIME = 'time', 'tottime'

    def __new__(cls, *values):
        obj = object.__new__(cls)
        # first value is canonical value
        obj._value_ = values[0]
        for other_value in values[1:]:
            cls._value2member_map_[other_value] = obj
        obj._all_values = values
        return obj

Operations like SortKey('file') will return SortKey.MODULE. The downside is that SortKey.FILE is undefined, but I think that is an acceptable trade-off since we are still supporting the original string values, and those values can be used to get a correct SortKey member.

With the new SortKey enum, these three lines are no longer needed.

When dealing with Enums, member is a better name than arg.

I don't understand -- 'file', 'filename', and 'module' should all create the same sort -- what ambiguity is there?

While what you're saying is essentially correct, however as it is currently implemented (without any of my changes), we're allowed to abbreviate the string argument into the sort_stats as long as the abbreviation is unambiguous. For example, sort_stats would work if either 'filename' or 'filena' string is passed in. But, if string 'file' is passed, then sort_stats can't differentiate between 'filename' or 'file' and sort_stats will fail with KeyError exception. We should probably remove 'file'. I can do that, if you agree. I'm guessing that no existing code is using 'file' argument at this time.

file and filename map to the same stat, so if we removed file and somebody passed in file it should still work.

Remove "file" and write a test to make sure it still works as intended.

Ok. Will make another pass to remove "file".

Since this is the string test, we should be iterating through all the possible strings (no longer a given with the new SortKey enum). Listing them all is probably the best way to go:

for arg in ('calls', 'ncalls', 'cumtime', 'cumulative', ... ):

Move SortKey.FILENAME up one line to the 'filename' entry, change the 'module' entry to N/A, and we're done! Thank you!