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

Typings and/or PHPDocs SettingsPiwik.php #16513

Merged
merged 4 commits into from Oct 12, 2020
Merged

Typings and/or PHPDocs SettingsPiwik.php #16513

merged 4 commits into from Oct 12, 2020

Conversation

tolbon
Copy link
Contributor

@tolbon tolbon commented Oct 2, 2020

No description provided.

@tsteur tsteur added the Needs Review PRs that need a code review label Oct 5, 2020
@tsteur
Copy link
Member

tsteur commented Oct 5, 2020

Thanks @tolbon Is there any particular reason for adding the typings and things we should look out for?

@tsteur
Copy link
Member

tsteur commented Oct 8, 2020

@tolbon be great if you could have a look at previous comment

@tolbon
Copy link
Contributor Author

tolbon commented Oct 8, 2020

Hi sorry for delay, I needed offscreen.
No no really, you target PHP 7.2. Php7.2 have typings...
It’s just a no critical class like Date. I think slowly move code with typing is a good things when you have big project like this.
Sometimes typing make appears some « bug » where PHPDoc say nothing. Like php docs say getSalt return a string but finally getSalt in test can return null (wrong test or wrong phpDocs ?).
I am totally open for suggestions/args 👍 I don’t have your vision or roadmap, Maybe you don’t target this (typing)

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Left 1 comment. I think otherwise the definitions look good.

core/SettingsPiwik.php Outdated Show resolved Hide resolved
@tsteur
Copy link
Member

tsteur commented Oct 12, 2020

Cheers @tolbon

@tsteur tsteur merged commit 065aca6 into matomo-org:4.x-dev Oct 12, 2020
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants