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

[2.14]: QA: Add specific type hints for $params #126

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 12, 2021

Q A
QA yes

Description

Extracted from #109

These need line length exception, which is difficult to setup, hence the separate PR to be able to work on the specific problem only.

@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

I just realized I could change Protocol\Pop3 -> Pop3 in import to save some bytes. This reduces code readability but "solves" the problem.

@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

aside, why "coverage/coveralls — Coverage remained the same at 62.166%" is an error? coverage must increase with each PR? :)

@Ocramius
Copy link
Member

Really don't know about the coveralls setup: I can imagine it erroring due to floating point comparisons.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM

@glensc
Copy link
Contributor Author

glensc commented Jan 12, 2021

LGTM

@Ocramius this can't be merged until Travis is fixed. It will continue to haunt all future PR's. The check runs on all code, not just modified lines.

I created the PR, so getting the ignore rules to work can be experimented, as I failed to find anything working in #109 (details in notes in there), and I understood you also tried something but failed as well.

@Ocramius
Copy link
Member

Probably better to merge #127 first then

@Ocramius Ocramius added this to the 2.13.0 milestone Jan 17, 2021
@glensc
Copy link
Contributor Author

glensc commented Jan 18, 2021

@Ocramius what exactly from there? can you point to a commit?

@Ocramius
Copy link
Member

@glensc since #127 and #117 are handled, could you run a rebase here, when you have time? In theory, that restores CI build status.

@Ocramius Ocramius modified the milestones: 2.13.0, 2.14.0 Jan 26, 2021
@Ocramius Ocramius changed the base branch from 2.13.x to 2.14.x January 26, 2021 13:41
@glensc
Copy link
Contributor Author

glensc commented Jan 26, 2021

@Ocramius seems you lost context, the CI failure is due to CS:

I created this PR out of #109 due unable to handle "line length cs violation", as the chunks did not get any resolution (so it got excluded from #109) nor any help on the matter as outlined in the PR body.

Please see the discussion in #109 it goes around the problem in rounds and rounds.

Here are direct links to each of the posts, but I find it easier to just read the whole PR notes top to bottom.

For this PR to go forward, need help with how to set up the CS violation exclude.

@Ocramius
Copy link
Member

@glensc
Copy link
Contributor Author

glensc commented Jan 26, 2021

Hmm, can't find Travis integration at all anymore.

And seems already master broken with test/Transport/SendmailTest.php from #117:

FILE: /Users/glen/scm/php/zend-mail/test/Transport/SendmailTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  97 | WARNING | Line exceeds 120 characters; contains 130 characters
 283 | WARNING | Line exceeds 120 characters; contains 122 characters
----------------------------------------------------------------------

Will send separate PR with CS fix disable

@glensc
Copy link
Contributor Author

glensc commented Jan 26, 2021

@Ocramius your suggestion is too complex, what to put under the name "Doctrine"?
Using value "Doctrine" obviously is not wanted, as that will enable ruleset named "Doctrine".

This project does include to vendor/laminas/laminas-coding-standard/ruleset.xml which has name="Zend Framework Coding Standard", using that results in a fatal error of ruleset not found.

anyway, found this page:

Property Name Type Default Available Since
lineLimit int 80 -
absoluteLineLimit int 100 -
ignoreComments bool false 3.1.0

what values to set here?

@glensc
Copy link
Contributor Author

glensc commented Jan 26, 2021

Submitted PR to up the limits: #132

@froschdesign
Copy link
Member

froschdesign commented Jan 27, 2021

@glensc
Based on your comment:

The phpcs:disable has no effect…

and the old PHP_CodeSniffer version, I think that the use is wrong. Try the old syntax:

// @codingStandardsIgnoreStart
protected function _detectCallbackUrl()
{
    // @codingStandardsIgnoreEnd
}

(laminas-coding-standard 1.0.0 uses version 2 of PHP_CodeSniffer but the new comments were added in version 3.2.0.)

@glensc
Copy link
Contributor Author

glensc commented Jan 27, 2021

@froschdesign ok. that makes a lot of sense! thanks for figuring that out.

but, what now?

  1. is there a plan for upgrading code standard in this project?
  2. just proceed with old syntax?
  3. ignore the problem?

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>
Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>
@glensc glensc changed the title Add specific type hints for $params [2.14]: QA: Add specific type hints for $params Feb 27, 2021
@glensc
Copy link
Contributor Author

glensc commented Feb 27, 2021

CI green now

@Slamdunk Slamdunk merged commit 785d2e8 into laminas:2.14.x Mar 15, 2021
@glensc glensc deleted the codestyle-types branch March 15, 2021 09:44
artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023
[2.14]: QA: Add specific type hints for $params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants