freezingsaddles / freezing-sync

Freezing Saddles activity retrieval and processing
Apache License 2.0
1 stars 1 forks source link

Keep display names stable #21

Closed merlinorg closed 4 years ago

merlinorg commented 4 years ago

Only update athlete display name if either they are a new athlete, or an existing athlete with a new athlete name and the old display name still matches the old athlete name. Closes #16. Hypothetical and untested, must learn to test strava sync.

obscurerichard commented 4 years ago

I refactored name sync code for safety and PEP-8.

I got rid of the super-verbose disambiguation code, the unambiguous_display_name function @merlinorg defined is superior and more concise.

I fixed one condition that would have caused an exception in that function though, we should not blow past the end of the string if the strings match. Two people named John Smith would have broken the code as it was written.

I did some exploratory testing with this code snipped to show that it should work:

exist_flag = False

def already_exists(name: str):
    return exist_flag

athlete_name  = "Ferd Berferd"

def unambiguous_display_name(idx: int) -> str:
    name = athlete_name[:idx]
    return name if name == athlete_name or \
        len (athlete_name) == idx or \
        not already_exists(name) \
        else unambiguous_display_name(idx + 1)

exist_flag=True
print(unambiguous_display_name(1))
exist_flag=False
print(unambiguous_display_name(1))

exist_flag=True
print(unambiguous_display_name(2 + athlete_name.index(' ')))
exist_flag=False
print(unambiguous_display_name(2 + athlete_name.index(' ')))

outputs:

Ferd Berferd
F
Ferd Berferd
Ferd B

We should really put some real unit tests in here...

It is hard to test the freezing-sync code safely in isolation right now, because you have to set up a Strava application and authorize a user. I haven't done it yet myself and this is year 2 I'm maintaining it. Without that the option is as follows:

image