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

Fixed error for Matomo (return code 500-php/server error) - no parent… #159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rristow
Copy link

@rristow rristow commented May 20, 2020

  • Fixed error for Matomo (return code 500-php/server error) - no parentheses in site-code'
  • Fixed the log-level (trace) for not configured search engines
  • Added the option to disable the tracking code

Sorry, no time to improve the tests

…heses in site-code'

Fixed the log-level (trace) for not configured search engines
Added the option to disable the tracking code
@bittner
Copy link
Member

bittner commented Jul 9, 2020

Your PR needs to be rebased to be eligible for being merged.

Sorry, no time to improve the tests

This sounds like an arrogant statement. I'm sure you don't mean it that way.

If your changes need tests to be adapted I'm afraid there is no other choice than getting that done, too. If you write tests, if you refactor tests, if you improve tests, that demonstrates professional attitude. If you break tests, that is something that would be unprofessional from our side to accept. We can't do that.

What we can do is offer our support. Let us know where you struggle with aligning the existing tests with your changes. There will be someone taking their time – even if "we don't have it", even when our families would deserve that time more. That's how we people are in the free software community. Ask us! We're happy about your contribution, and we're here to help.

@bittner
Copy link
Member

bittner commented Dec 5, 2020

Any chance you would rebase your branch for this PR, @rristow?

If you need any help for adjusting the tests afterwards, please let us know!

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

This looks like a valuable contribution. Would you still be interested to take this forward?

Comment on lines 13 to +14
from analytical.utils import get_identity, is_internal_ip, disable_html, \
get_required_setting

get_required_setting
Copy link
Member

Choose a reason for hiding this comment

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

How about?

from analytical.utils import (
    disable_html,
    get_identity,
    get_required_setting,
    is_internal_ip,
)

Comment on lines 17 to +19
from analytical.utils import disable_html, get_required_setting, \
is_internal_ip, get_user_from_context, get_identity, \
get_user_is_authenticated
is_internal_ip, get_user_from_context, get_identity, \
get_user_is_authenticated
Copy link
Member

Choose a reason for hiding this comment

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

Don't isort and black like something like this better?

from analytical.utils import (
    disable_html,
    get_identity,
    get_required_setting,
    get_user_from_context,
    get_user_is_authenticated,
    is_internal_ip,
)

Comment on lines 13 to +14
from analytical.utils import is_internal_ip, disable_html, get_identity, \
get_required_setting

get_required_setting
Copy link
Member

Choose a reason for hiding this comment

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

Same as commented above.

Comment on lines 12 to +13
from analytical.utils import is_internal_ip, disable_html, \
get_required_setting

get_required_setting
Copy link
Member

Choose a reason for hiding this comment

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

Same as commented above.

Comment on lines 12 to +13
from analytical.utils import get_identity, is_internal_ip, disable_html, \
get_required_setting

get_required_setting
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 13 to +14
from analytical.utils import is_internal_ip, disable_html, \
get_required_setting

get_required_setting
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

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.

2 participants