Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetTitle WorkerHelper function doesn't properly escape all invalid filename characters (Hint: Don't take the *NSYNC peloton class) #430

Closed
YakubYo opened this issue Jan 26, 2023 · 2 comments · Fixed by #431
Assignees

Comments

@YakubYo
Copy link

YakubYo commented Jan 26, 2023

Describe the bug
File IO exception if you take Cody Rigsby's 30 minute *NSYNC class from Nov 2022 and then run Peloton-to-Garmin console app. :) Writing the FIT file locally causes an error.

To Reproduce

  1. Enjoy cheesy boy-band music while you ride (or choose the class if you've run out of most other options)
  2. Run console app
  3. Boom

Note:
The issue appears to be that the GetTitle helper function only strips out two of the most common invalid filename characters...

		return $"{rideTitle}{instructorName}"
			.Replace(" ", "_")
			.Replace("/", "-")
			.Replace(":", "-");

Recommend this be replaced with the .NET function to return full set of invalid chars to strip... this worked for me locally:

		string fullTitle = $"{rideTitle}{instructorName}";
		foreach (char invalidChar in Path.GetInvalidFileNameChars())
		{
			fullTitle = fullTitle.Replace(invalidChar, '-');
		}

		return fullTitle.Replace(" ", "_");

Note 1: Running version 3.3.1 of console app on a Windows box
Note 2: Just got this app and otherwise love it! Thanks!

@YakubYo YakubYo added the bug label Jan 26, 2023
@philosowaffle
Copy link
Owner

Good find, good suggestion. Day job is killing me right now, but hopefully will have some time to get this fix in on the weekend.

@YakubYo
Copy link
Author

YakubYo commented Feb 4, 2023

Good find, good suggestion. Day job is killing me right now, but hopefully will have some time to get this fix in on the weekend.

Thank for incorporating this fix! This is a fantastic app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants