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

Complete the unit testing and add some features #93

Closed
wants to merge 5 commits into from

Conversation

peter279k
Copy link
Contributor

First of all, thank you for your great package. Let me access OpenWeatherMap API with PHP easily.

Change log

  • Complete the unit testing.

  • Ignore testing the getWeatherHistory method in OpenWeatherMap.php.
    This method is ignored because the api key need to have a paid permission.

  • Integrate the integTests with tests.

  • Replace sprintf function with str_replace in Weather.php because the character %s is proper for using the sprintf.

  • Replace json_last_error_msg with json_last_error in OpenWeatherMap.php. Because the json_last_error_msg is available for PHP 5.5+ (see this link). In order to be compatible with older PHP version (like 5.3 or 5.4), so we do this.

  • In ExampleCache.php, adding setTempPath method allow us to specify the cached file path.

  • Using the # to comment message is deprecated in *.ini file. Replace the # with this ;.

  • Set the proper configuration in phpunit.xml.dist file.

  • Add the required extension: ext-curl in composer.json.
    It will check whether the curl extension is installed during running the Composer installation.

  • Add the require-dev key in composer.json.

the travis-ci result
the testing code coverage report (provided by codecov)

@codecov-io
Copy link

codecov-io commented Dec 30, 2016

Current coverage is 97.82% (diff: 100%)

No coverage report found for master at dadc126.

Powered by Codecov. Last update dadc126...fd923d7

@peter279k
Copy link
Contributor Author

Hi @cmfcmf , I fix the icoUrl variable error.

Copy link
Owner

@cmfcmf cmfcmf left a comment

Choose a reason for hiding this comment

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

Hey @peter279k,
thank you for your contributions! I've gone through most of your changes now and only found a few minor issues which I have already fixed myself. Expect your changes to be merged soon!

@@ -75,6 +75,9 @@ public function __construct($weatherHistory, $query)

$utctz = new \DateTimeZone('UTC');
foreach ($weatherHistory['list'] as $history) {
if (isset($history['city'])) {
continue;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you add this?

Copy link
Contributor Author

@peter279k peter279k Jan 2, 2017

Choose a reason for hiding this comment

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

Hi @cmfcmf , I didn't add this.
When I pull your repo, it automatically add this.Perhaps I don't remember to add this.
When I remove this code, some error will happen during unit testing.

Hi @cmfcmf , thank you for your comment.
Finally I found where the problem is. I will remove this later.

@cmfcmf
Copy link
Owner

cmfcmf commented Jan 2, 2017

This has been merged as part of #96. Thank you @peter279k!

@cmfcmf cmfcmf closed this Jan 2, 2017
@peter279k peter279k mentioned this pull request Jan 6, 2017
21 tasks
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