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

Feature: acquire ublaboo #12

Merged
merged 1 commit into from Apr 22, 2018
Merged

Feature: acquire ublaboo #12

merged 1 commit into from Apr 22, 2018

Conversation

@f3l1x
Copy link
Member

@f3l1x f3l1x commented Apr 11, 2018

Refactoring:

  • require PHP 7
  • update codesniffer
  • update ruleset (TRUE=>true)
  • use Travis CI stages
  • update readme
  • added @paveljanda to authors
@f3l1x f3l1x requested a review from paveljanda Apr 11, 2018
@f3l1x f3l1x added this to the v0.3 milestone Apr 11, 2018
@f3l1x f3l1x force-pushed the feature/ublaboo branch from e216312 to dc78a96 Apr 11, 2018
@@ -0,0 +1,121 @@
<?php declare(strict_types = 1);
Copy link
Member

@paveljanda paveljanda Apr 12, 2018

What are the conventions in opensource php community? In Gamee, Monday Factory, we use:

<?php

declare(strict_types=1);

So does for example consistence/consistence.

Copy link
Member Author

@f3l1x f3l1x Apr 12, 2018

It's based on slevomat/coding-standards. :(

Copy link
Member

@paveljanda paveljanda Apr 19, 2018

Right, ok.

Copy link

@Majkl578 Majkl578 Jun 17, 2018

It's based on slevomat/coding-standards. :(

Just for reference, this statement is incorrect - Slevomat CS is only a set of rules and this sniff is configurable. Doctrine also uses Slevomat CS with no spaces around = and two newlines after open tag:
https://github.com/doctrine/coding-standard/blob/0469c18a1a4724c278f2879c0dd7b1fa860b52de/lib/Doctrine/ruleset.xml#L192-L208

class CSVResponse implements IResponse
{

/** @var string */
Copy link
Member

@paveljanda paveljanda Apr 12, 2018

Personally, I don't like these short docblocks. In all project we use:

/**
 * @var string
 */
protected $delimiter;

And so does Symfony, Guzzle, etc.

Copy link
Member Author

@f3l1x f3l1x Apr 19, 2018

We agreed with @paveljanda to write it both ways. We'll see in the future.

)
{
if (strpos($name, '.csv') === FALSE) {
$name = sprintf('%.csv', $name);
Copy link
Member

@paveljanda paveljanda Apr 12, 2018

Maybe $name = sprintf('%s.csv', $name); to make more clear?

Copy link
Member Author

@f3l1x f3l1x Apr 12, 2018

Yep, it's a bug.

Copy link
Member Author

@f3l1x f3l1x Apr 19, 2018

Fixed.

@paveljanda
Copy link
Member

@paveljanda paveljanda commented Apr 12, 2018

We should also change FALSE to false, TRUE to true and NULL to null.. :)

Refactoring:
- require PHP 7
- update codesniffer
- update ruleset (TRUE=>true)
- use Travis CI stages
- update readme
- added @paveljanda to authors
@f3l1x
Copy link
Member Author

@f3l1x f3l1x commented Apr 19, 2018

I've fixed mentioned bug and convert TRUE=>true. Take a look @paveljanda.

@f3l1x f3l1x force-pushed the feature/ublaboo branch from dc78a96 to 6628d04 Apr 19, 2018
* @return string
* @throws InvalidLinkException
* @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingParameterTypeHint
* @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingReturnTypeHint
*/
public function link($dest, array $params = []);
Copy link
Member

@paveljanda paveljanda Apr 19, 2018

string $dest?

Copy link
Member Author

@f3l1x f3l1x Apr 20, 2018

I would like, but it's not compatible with Nette LinkGenerator, thus there is phpcsSupress ;-)

*/
public function link($dest, array $params = [])
public function link($dest, array $params = []): string
Copy link
Member

@paveljanda paveljanda Apr 19, 2018

string $dest?

@@ -0,0 +1,121 @@
<?php declare(strict_types = 1);
Copy link
Member

@paveljanda paveljanda Apr 19, 2018

Right, ok.

@paveljanda
Copy link
Member

@paveljanda paveljanda commented Apr 19, 2018

My last comments, otherwise it's ok. :)

@f3l1x f3l1x merged commit e683422 into master Apr 22, 2018
3 checks passed
@f3l1x f3l1x deleted the feature/ublaboo branch Apr 22, 2018
@f3l1x
Copy link
Member Author

@f3l1x f3l1x commented Apr 22, 2018

Welcome @paveljanda ;-)

@f3l1x f3l1x mentioned this pull request Apr 22, 2018
@paveljanda
Copy link
Member

@paveljanda paveljanda commented Apr 23, 2018

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants