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

Compatible with Django-Pipeline >= 1.6.x, fixed issue with Permission… #16

Merged
merged 3 commits into from
Sep 10, 2016
Merged

Conversation

solkaz
Copy link
Contributor

@solkaz solkaz commented Aug 29, 2016

  • Commands that are passed int execute_command are now tuples instead of strings, which is what Django-Pipeline expects.
  • Fixed an issue that was causing PermissionErrors; the first element in command cannot be an empty string, so it will check for that. Becomes unnecessary after this PR is merged
  • Fixed the is_outdated method, as output isn't returned from execute_command so it's captured as stdout_captured
  • Simplified some of the logic

@j0hnsmith
Copy link
Owner

Looks great, does this mean that django-pipeline-browserify will only be compatible with specific versions of django-pipeline (eg 1.6+)? Does README.rst need to be updated?

@solkaz
Copy link
Contributor Author

solkaz commented Aug 30, 2016

I believe so, this commit had changed command to pass a tuple to execute_command, because shell=True was removed from subprocess.Popen. There's also this commit that places all pipeline settings into a dictionary. So, I would say that it's compatible with 1.6+, and yes, the readme needs to be updated.

if exists == True or exists == False:
deps.append(dep['file'])

if command[0] is '':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strings should be compared by value, not reference, so this should be if command[0] == '':.

@frewsxcv
Copy link
Collaborator

For the record, jazzband/django-pipeline#590 just merged.

@solkaz
Copy link
Contributor Author

solkaz commented Aug 30, 2016

Addressed the comparison issues @frewsxcv

@solkaz
Copy link
Contributor Author

solkaz commented Aug 30, 2016

Removed the check for an empty first argument as the PR in Django-Pipeline was merged.

@frewsxcv
Copy link
Collaborator

Removed the check for an empty first argument as the PR in Django-Pipeline was merged.

Considering a new django-pipeline release hasn't happened yet, does it make sense to remove it yet?

@solkaz
Copy link
Contributor Author

solkaz commented Aug 30, 2016

Yeah, that doesn't make sense. I've removed the commit to keep the check

@frewsxcv
Copy link
Collaborator

frewsxcv commented Sep 2, 2016

@solkaz New django-pipeline version is out, so that commit could be added again ;)

jazzband/django-pipeline@b0f83b9

@frewsxcv
Copy link
Collaborator

frewsxcv commented Sep 2, 2016

Could also bump the required version in setup.py to 1.6.9 if we're going to rely on the 1.6.9 change

@solkaz
Copy link
Contributor Author

solkaz commented Sep 2, 2016

Adding it back now 🎉 Will also update setup.py

@solkaz
Copy link
Contributor Author

solkaz commented Sep 2, 2016

Both commits have been uploaded

@solkaz
Copy link
Contributor Author

solkaz commented Sep 9, 2016

@j0hnsmith could you merge this now that django-pipeline has been updated?

@frewsxcv
Copy link
Collaborator

frewsxcv commented Sep 9, 2016

Alternatively, if you're looking for help from volunteers like myself or solkaz for maintaining this awesome project, let us know! I'd be happy to help!

@j0hnsmith j0hnsmith merged commit 18eeab6 into j0hnsmith:master Sep 10, 2016
@j0hnsmith
Copy link
Owner

Thanks @solkaz

@j0hnsmith
Copy link
Owner

@frewsxcv Thanks for the offer, I've invited you to be a collaborator.

@solkaz solkaz deleted the solkaz branch September 10, 2016 20:02
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