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

update-deps: Fix DiffHighlight's URL and refactor #260

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

oranja
Copy link
Contributor

@oranja oranja commented Aug 14, 2017

Seems like I'm the only one who tries to use update-deps.sh, huh?
This time there were two different problems:

  1. diff-highlight was made a .pm so the old URL doesn't point to a valid endpoint.
  2. diff-highlight/DiffHighlight is no longer expected to be found locally in third-party, but in lib. I guess that's some perl or at least fatpack convention for where local modules should be placed.

A word of warning, though: Unlike this repo's version of DiffHighlight , the current git-contrib version of DiffHighlight doesn't use Encode.
This means that after an update & rebuild (fatpack), diff-so-fancy will fail (here) unless it has its own use Encode.

Source: It bit me. :/

@scottchiefbaker
Copy link
Contributor

I appreciate you giving update-deps.sh some love. It doesn't get a lot of attention.

  1. You're absolutely correct
  2. Previously diff-highlight was a stand alone script that we shelled out to run. It's been made in to a module for performance improvements. This is not a fatpack thing, but a general Perl thing.

fatpack is now used to "bundle" the script and the DiffHighlight.pm dependency in to one script for simplicity. Can you elaborate on the git-contrib piece, I don't totally follow what you're trying to convey.

scottchiefbaker added a commit to scottchiefbaker/diff-so-fancy that referenced this pull request Aug 14, 2017
@scottchiefbaker
Copy link
Contributor

The use Encode thing is weird... it's included in DiffHighlight.pm so the main script should be able to access those functions also. I did update the main script to explicitly call it anyway.

@oranja
Copy link
Contributor Author

oranja commented Aug 14, 2017

You already fixed it, but I'll try to be clearer.

In this repo (diff-so-fancy), DiffHighlight.pm has a use Encode; line that doesn't exist in DiffHighlight.pm from the official Git repository (in contrib/diff-highlight).

The base diff-so-fancy script (before fatpack-ing), calls Encode::encode() but doesn't have a use Encode; line.

If I don't run update-deps.sh, the final fatpacked version has a use Encode; line, as you said, and everything is OK,
but if I do run update-deps.sh, the final fatpacked version won't have this line anymore and it will exit with an error.

Your fix (#260) prevents the error.

Edit: I see the error from Travis. I will fix it soon.

@scottchiefbaker
Copy link
Contributor

Oh! Upstream DiffHighlight.pm makes more sense. I've diverged slightly from upstream. I submitted some changes to @peff but he's been pretty busy.

I'll gladly merge this, if you change it to be against next. master is stable and released, anything other than emergency bugfixes should be directed against next.

Fetch `DiffHighlight.pm` instead of `diff-highlight`
@oranja oranja changed the base branch from master to next August 17, 2017 07:02
@oranja
Copy link
Contributor Author

oranja commented Aug 17, 2017

Rebased upon next.

Upstream. I should have just said upstream 🤓

@scottchiefbaker scottchiefbaker merged commit 799e23a into so-fancy:next Aug 17, 2017
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