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

fix: shell quote file names that contain spaces when printing them to stdout #51

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

fredmorcos
Copy link
Contributor

What the title says.

@alexpasmantier
Copy link
Owner

I believe this should be implemented here instead.

    pub fn stdout_repr(&self) -> String {
        let mut repr = self.name.clone();
        if repr.contains(|c| char::is_ascii_whitespace(&c)) {
            ...
        }
        if let Some(line_number) = self.line_number {
            repr.push_str(&format!(":{line_number}"));
        }
        repr
    }

It's fine to work with paths that contain spaces inside the application, but you're right, we do want to quote them whenever we need to write out to the shell etc.

@fredmorcos
Copy link
Contributor Author

I believe this should be implemented here instead.

I made the change. I had to force push the updated commit, I don't know how you feel about force pushes during reviews.

@alexpasmantier
Copy link
Owner

Thanks for the update 🙏🏻

Merging this 👍🏻

@alexpasmantier alexpasmantier changed the title Shell quote file names with spaces in them fix: shell quote file names that contain spaces when printing them to stdout Nov 20, 2024
@alexpasmantier alexpasmantier merged commit 21cdaae into alexpasmantier:main Nov 20, 2024
3 checks passed
@fredmorcos fredmorcos deleted the quote-filenames branch November 26, 2024 06:29
@fredmorcos
Copy link
Contributor Author

I apologize. This completely breaks Television because even normal commands (with spaces between command name and arguments) are output with quotes:

'docker start -ai dc-ubuntu24 '
bash: docker start -ai dc-ubuntu24 : command not found

How would you like to revert it; should I open a PR or do you want to do it some other way?

@alexpasmantier
Copy link
Owner

I noticed that yesterday too. No worries, will push a fix in the morning.

@alexpasmantier
Copy link
Owner

Fixed by #77

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

Successfully merging this pull request may close these issues.

2 participants