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

Expect drive letter only on vanilla windows #8227

Merged
merged 1 commit into from Jun 5, 2020

Conversation

ashmaroli
Copy link
Member

Summary

Bash on Windows expands similar to paths on Linux

Context

Based on discussion in #8220 with @noproblema
Resolves #8220

Bash on Windows expands similar to paths on Linux
@ashmaroli ashmaroli added the tests label Jun 3, 2020
@ashmaroli
Copy link
Member Author

@noproblema This patch should resolve your issue, To test at your end, run the following:

git fetch upstream refs/pull/8227/head:vanilla-windows-temp-dirpath
git checkout vanilla-windows-temp-dirpath
bash script/test test/test_site.rb

Let me know if the test passes on your end now.

@noproblema
Copy link

noproblema commented Jun 3, 2020 via email

@ashmaroli ashmaroli requested a review from DirtyF June 3, 2020 15:40
@ashmaroli
Copy link
Member Author

Thank you for testing @noproblema
Once again, I'd like to inform you that we're not comparing C:/tmp with /tmp, but that we're testing if the resulting path will be C:/tmp/whatever on Windows and /tmp/whatever on Unix-based platforms.

@ashmaroli ashmaroli removed the request for review from DirtyF June 5, 2020 07:04
@ashmaroli
Copy link
Member Author

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 75ae986 into jekyll:master Jun 5, 2020
@ashmaroli ashmaroli deleted the vanilla-windows-temp-dirpath branch June 5, 2020 07:05
jekyllbot added a commit that referenced this pull request Jun 5, 2020
@noproblema
Copy link

noproblema commented Jun 5, 2020 via email

@ashmaroli
Copy link
Member Author

@noproblema You yourself told me a couple of days ago that with the proposed change here, the tests pass successfully on your computer. Ergo, the tests should pass with the latest master now that this has been merged.

I will clarify it for you one last time and continued queries will be ignored.

The test code you're referring to is:

jekyll/test/test_site.rb

Lines 39 to 45 in 81c1350

should "have an array for plugins if passed as an array" do
site = Site.new(site_configuration(
"plugins_dir" => ["/tmp/plugins", "/tmp/otherplugins"]
))
array = [temp_dir("plugins"), temp_dir("otherplugins")]
assert_equal array, site.plugins
end

The fixture site used in the test is configured to point to an array of paths. The config translates to the following YAML data (simulating what the hypothetical user has given Jekyll):

plugins_dir:
- "/tmp/plugins"
- "/tmp/otherplugins"

On Unix based platforms (and apparently on Bash on Windows, too), Jekyll processes the above configuration as Ruby array:

["/tmp/plugins", "/tmp/otherplugins"]

On pure Windows platform, Jekyll processes the above configuration into Ruby array based on the current drive the user's source directory is:

["C:/tmp/plugins", "C:/tmp/otherplugins"]

What the test-helper method temp_dir() does is to take the given strings and join them to <root>/tmp/. Agreed that it is not a very robust and flexible implementation. But it serves the purpose.

Therefore, the code [temp_dir("plugins"), temp_dir("otherplugins")] will evaluate into ["C:/tmp/plugins", "C:/tmp/otherplugins"] and ["/tmp/plugins", "/tmp/otherplugins"] on pure Windows platforms and on other platforms respectively. The test passes on all platforms because the test helper adapts results based on the platform (which mirrors the value of site.plugins on that platform).

If it is still not clear, you probably need to brush up on your Ruby and programming skills.

@jekyll jekyll locked and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants