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

Change $_ENV variable handling #115

Closed
wants to merge 6 commits into from
Closed

Change $_ENV variable handling #115

wants to merge 6 commits into from

Conversation

tarlepp
Copy link
Contributor

@tarlepp tarlepp commented Dec 7, 2017

Fixes #114

@DonCallisto
Copy link
Collaborator

Umh, first of all, you should stash this into a single commit (apart PHP 7.2 that should not be into this PR as is a totally different concept.

Moreover if you rebase this to dev, you should have already this.

Second thing: has this part a test? If not, could you add some test here? That's because when we have all test matrix running we can check if everything works even with previous versions

@tarlepp
Copy link
Contributor Author

tarlepp commented Dec 8, 2017

@DonCallisto I runned all the tests that library currently contains without any problems - so I assumed that would be enough. Going to close this one for now, perhaps someone else have some time to make necessary changes - unfortunately currently I haven't :(

btw, we should add some contributor guidelines so that people would know what are expected with pull requests and what not.

@tarlepp tarlepp closed this Dec 8, 2017
@alexislefebvre
Copy link
Collaborator

@DonCallisto IIRC we have an option to squash commits when merging a PR.

@DonCallisto
Copy link
Collaborator

DonCallisto commented Dec 9, 2017

@tarlepp yes you would be right as long as we have the code covered by tests; I'm not sure this is the case, so we should at least try to think if further tests are needed or not.
First thing you can try to do is to rebase this with #116 and see if all tests with that matrix are good.
If they're good, then let's see together if test are needed and someone will do them (if you can't due to your time and schedule).

btw, we should add some contributor guidelines so that people would know what are expected with pull requests and what not.

I agree with this

@alexislefebvre I forgot that we can squash them, however it was only an advice just to set a "standard" for PRs but as @tarlepp said, we should write some guidelines.

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