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

consider as uri if it is not a resource #20

Merged
merged 3 commits into from
Nov 9, 2018
Merged

consider as uri if it is not a resource #20

merged 3 commits into from
Nov 9, 2018

Conversation

Natkeeran
Copy link
Contributor

GitHub Issue: (link)
Islandora/documentation#943

What does this Pull Request do?

This PR will tell Crayfish-Commons to not to load $data into the pipe if it is not a resource. It is assumed that a resource url will be passed in. Specifically to address the above 943 ffmpeg issue.

How should this be tested?

This needs to be tested other related PRs. Which I will post shortly here.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora-CLAW/committers

@dannylamb
Copy link
Contributor

phpcbf can take care of Travis: https://travis-ci.com/Islandora-CLAW/Crayfish-Commons/jobs/156095121

@dannylamb
Copy link
Contributor

linking to Islandora/Crayfish#52

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #20 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #20      +/-   ##
============================================
- Coverage     78.91%   78.67%   -0.24%     
- Complexity      109      110       +1     
============================================
  Files             8        8              
  Lines           332      333       +1     
============================================
  Hits            262      262              
- Misses           70       71       +1
Impacted Files Coverage Δ Complexity Δ
src/CmdExecuteService.php 4.76% <0%> (-0.12%) 13 <0> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209117a...a0b07f7. Read the comment docs.

Copy link
Contributor

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

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

Shouldn't hurt anything to pull this in now. Let's do it 👍

@dannylamb dannylamb merged commit b07197c into Islandora:master Nov 9, 2018
@dannylamb
Copy link
Contributor

Codecov forced me to use admin privileges to merge that because coverage dropped by a tiny percentage. I should go through our repos and disable that so it's not a 'required' status check and instead just a tool of guilt.

@dannylamb
Copy link
Contributor

Eww... so I don't know if I can do that...

It's either required or not a status check at all. I wonder if it will still run when not a status check? Only one way to find out.

@whikloj
Copy link
Member

whikloj commented Nov 13, 2018

It won't run, we should keep it as required and figure out why coverage dropped and add tests so it doesn't.

@whikloj
Copy link
Member

whikloj commented Nov 13, 2018

Actually we have no tests for the CmdExecuteService so adding any complexity to it reduces the overall coverage. We should add tests for this service.

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