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

THRIFT-4423 migrate to psr-4 #1445

Closed
wants to merge 4 commits into from
Closed

Conversation

robberphex
Copy link
Contributor

@robberphex robberphex commented Dec 13, 2017

@robberphex robberphex changed the title migrate to psr-4 THRIFT-4423: migrate to psr-4 Dec 13, 2017
@robberphex robberphex changed the title THRIFT-4423: migrate to psr-4 THRIFT-4423 migrate to psr-4 Dec 13, 2017
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Good to see it all passed tests. The one issue I think really needs to be fixed or explained is putting a command in the docker run file. It may not have done what you expected it to.

@@ -25,6 +25,8 @@ DOCKER_TAG=$DOCKER_REPO:$DISTRO

printenv | sort

composer install
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be part of the php section of the Dockerfile. By putting it in the run section, we run "composer install" on the host and then start a docker environment which doesn't have access to it.

@@ -56,7 +56,3 @@ echo TODOs: `grep -r TODO * | wc -l`

# LoC
sloccount .

# System Info
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing these lines looks unrelated to the task at hand, and they don't take much time, and they provide useful info.

@@ -17,6 +17,8 @@
# under the License.
#

PHPUNIT=./../../../vendor/bin/phpunit
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this relative path going to work on every platform?

@jeking3
Copy link
Contributor

jeking3 commented Jan 11, 2018

I'll watch for a passing build, and then merge tomorrow if it's good.

@jeking3
Copy link
Contributor

jeking3 commented Jan 11, 2018

That pesky concurrency test! Not related to this.

@asfgit asfgit closed this in a15060a Jan 11, 2018
@robberphex robberphex deleted the psr-4 branch January 15, 2018 11:13
@robberphex robberphex restored the psr-4 branch January 15, 2018 11:13
@robberphex robberphex deleted the psr-4 branch January 15, 2018 11:13
@robberphex robberphex restored the psr-4 branch January 15, 2018 11:13
@robberphex robberphex deleted the psr-4 branch January 15, 2018 12:32
jeking3 pushed a commit to jeking3/thrift that referenced this pull request Mar 10, 2018
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