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

Support spaces in app path #322

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

kukicola
Copy link
Contributor

Summary

Hello, when upgrading my app from v6 to v7, I noticed some commands are failing because I have a space in the absolute path of my project.

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Other Information

@ahangarha
Copy link
Contributor

May you create a minimal app, demonstrating the issue?

@kukicola
Copy link
Contributor Author

kukicola commented Jul 2, 2023

Easiest way to replicate the issue is to create a new app in the folder with space (in this case test with space), add shakapacker, and execute shakapacker:install task, which will fail:

➜  test with space pwd
/Users/kukicola/Desktop/test with space
➜  test with space rails new . --skip-javascript
       exist  
      create  README.md
      create  Rakefile
      create  .ruby-version
      create  config.ru
      ...

➜  test with space git:(main) ✗ bundle add shakapacker --strict
Fetching gem metadata from https://rubygems.org/..........
Resolving dependencies...
Fetching gem metadata from https://rubygems.org/..........
...

➜  test with space git:(main) ✗ ./bin/rails shakapacker:install
/Users/kukicola/Desktop/test: --> /Users/kukicola/Desktop/test
syntax error, unexpected integer literal, expecting end-of-input

@ahangarha
Copy link
Contributor

Do you really need to have your app in a directory with such name? This is not considered a good practice to name files or directories with spaces.

@justin808
Copy link
Member

@ahangarha please test this and I'll ship 7.0.3 with this. Please update the changelog.

Copy link
Contributor

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

I tested your changes and it works as expected.
I added few comments. Take a look at them please.

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to support spaces in the path for tests, there are more cases to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests are currently passing, only this one had any issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough
@justin808 This PR looks good to me.

@ahangarha
Copy link
Contributor

@kukicola
Please rebase your branch on the latest changes on our master branch.

@justin808
Except for Changelog, this PR looks good to me.

@ahangarha
Copy link
Contributor

@kukicola I don't think you did it in the right way.
If you don't mind, I will bring your commits and create a new PR and fix the changelog.

@justin808 do you have any other consideration?

@justin808 justin808 merged commit b8ffa3d into shakacode:master Jul 7, 2023
@justin808
Copy link
Member

@ahangarha I merged. You can add another commit and I can release a new version.

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.

3 participants