there4 / markdown-resume

Generate a responsive CSS3 and HTML5 resume with Markdown, with optional PDF output.
MIT License
1.77k stars 519 forks source link

fixes the ternary operator to set the correct autoloadPath #79

Closed johnpneumann closed 6 years ago

johnpneumann commented 6 years ago

resolves #77.

The issue with the fix proposed in #66 is that the markdown-resume directory will always be within the path even when it's added as a package in its own composer file. As such, it means that the first argument in the ternary operator will always return.

If you want to test this:

echo "Testing as a normal install"
cd $(mktemp -d)
git clone https://github.com/johnpneumann/markdown-resume.git normal
cd normal
git checkout 77_fix_autoload_path
composer install
bin/md2resume
cd ..
echo "Testing as a dependency"
mkdir as-dependency
cd as-dependency
cat > composer.json <<EOF
{
    "name": "md2resume-test",
    "minimum-stability": "dev",
    "repositories": [
        {
            "type": "path",
            "url": "../normal"
        }
    ],
    "require": {
        "php": ">=7.1",
        "there4/markdown-resume": "*"
    }
}
EOF
composer install
vendor/bin/md2resume
f=$(egrep '^\$autoloadPath' vendor/bin/md2resume)
g=$(egrep '^\$autoloadPath' ../normal/bin/md2resume)
[[ "${f}" == "${g}" ]] && echo "autoloadPaths match"

Hopefully that's clear as mud. I haven't written PHP in a decade and have no experience with composer outside of today, so if there's a better way for this, feel free to correct.

spawnia commented 6 years ago

This looks nice, does this solve #70 and #71 as well by any chance?

Love the test script, do you think it can be included into travis?

asbjornu commented 6 years ago

LGTM. With the test script included in Travis, this would get my thumb up. 👍