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

DX: Demote loglevel of message on url parameters to DEBUG while guessing RI #6891

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

yarikoptic
Copy link
Member

It is #6890 by @adswa with my addition to the solution -- log at DEBUG only while guessing. See the last commit

Our own obscure paths generate filepaths that RFC 1808 on URLs
schemes mistakes to have URL parameter specifications because
they contain a semicolon (see #6872).
Internally, when urlparse detects parameters in a URL, we warn
that those parameters will not be honored or considered at all.
In general, I find this is a nice thing to do, but annoying when
its actually not a URL with parameters. My initial thought on
how to keep the information but make it less annoying is to
demote the log level of the message from warning to debug.
Alternatively, we could do more parsing and guessing on whether
or not something a URL where parameters make sense, but that
could probably easily introduce more problems or wrong assessments. So this change here could maybe fix #6872

Changelog

🏠 Internal

  • In order to reduce inconsequential warnings over obscure paths resembling URLs with parameters in tests, a warning about ignoring URL parameters has been demoted to debug log level

adswa and others added 3 commits July 26, 2022 18:13
Our own obscure paths generate filepaths that RFC 1808 on URLs
schemes mistakes to have URL parameter specifications because
they contain a semicolon (see  datalad#6872).
Internally, when urlparse detects parameters in a URL, we warn
that those parameters will not be honored or considered at all.
In general, I find this is a nice thing to do, but annoying when
its actually not a URL with parameters. My initial thought on
how to keep the information but make it less annoying is to
demote the log level of the message from warning to debug.
Alternatively, we could do more parsing and guessing on whether
or not something a URL where parameters make sense, but that
could probably easily introduce more problems or wrong assessments.
…to _pr_to_fields

That "guessing" would add semantic on the purpose of the invocation
of that function, and if we are guessing only, there is no need
to scarry user with the warning.  But if it is invoked in the other
use case where it is to parse URL (e.g. having decided it is a URL),
then it would still issue warning
@yarikoptic yarikoptic added the semver-internal Changes only affect the internal API label Jul 26, 2022
@yarikoptic
Copy link
Member Author

Attn @christian-monch , something new with metalad causing the

2 errors which are unlikely to be triggered by changes here?
======================================================================
ERROR: datalad_metalad.tests.test_locking.test_meta_add_locking_end_to_end
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/runner/work/datalad/datalad/datalad/tests/utils_pytest.py", line 336, in _wrap_skip_if
    pytest.skip(msg if msg else "condition was True")
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/_pytest/outcomes.py", line 175, in skip
    raise Skipped(msg=reason, allow_module_level=allow_module_level)
Skipped: condition was True

======================================================================
ERROR: datalad_metalad.tests.test_locking.test_meta_add_locking_impact_end_to_end
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/runner/work/datalad/datalad/datalad/tests/utils_pytest.py", line 336, in _wrap_skip_if
    pytest.skip(msg if msg else "condition was True")
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/_pytest/outcomes.py", line 175, in skip
    raise Skipped(msg=reason, allow_module_level=allow_module_level)
Skipped: condition was True

@christian-monch
Copy link
Contributor

Attn @christian-monch , something new with metalad causing the

2 errors which are unlikely to be triggered by changes here?

======================================================================
ERROR: datalad_metalad.tests.test_locking.test_meta_add_locking_end_to_end
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/runner/work/datalad/datalad/datalad/tests/utils_pytest.py", line 336, in _wrap_skip_if
    pytest.skip(msg if msg else "condition was True")
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/_pytest/outcomes.py", line 175, in skip
    raise Skipped(msg=reason, allow_module_level=allow_module_level)
Skipped: condition was True

======================================================================
ERROR: datalad_metalad.tests.test_locking.test_meta_add_locking_impact_end_to_end
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/runner/work/datalad/datalad/datalad/tests/utils_pytest.py", line 336, in _wrap_skip_if
    pytest.skip(msg if msg else "condition was True")
  File "/opt/hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/_pytest/outcomes.py", line 175, in skip
    raise Skipped(msg=reason, allow_module_level=allow_module_level)
Skipped: condition was True

Thx, should have been fixed in PR #272 of datalad-metalad

@yarikoptic
Copy link
Member Author

ok, let's proceed!

@yarikoptic yarikoptic merged commit 825c8e0 into datalad:maint Jul 29, 2022
@yarikoptic
Copy link
Member Author

eh -- I have not looked into appveyor fails, one of the windows envs did have it "related":
https://ci.appveyor.com/project/mih/datalad/builds/44287617/job/fgp764p65r1ba5oj

____________________________ test_pathri_guessing _____________________________
filename = 'C:\\DLTMP\\datalad_temp_pwl4puuu'
    @with_tempfile
    def test_pathri_guessing(filename=None):
        # Complaining about ;param only at DEBUG level
        # see https://github.com/datalad/datalad/issues/6872
        with swallow_logs(new_level=logging.DEBUG) as cml:
            # we don't "care" about params ATM so there is a warning if there are any
            ri = RI(f"{filename};param")
            assert isinstance(ri, PathRI)
>           assert_in('ParseResults contains params', cml.out)
..\datalad\support\tests\test_network.py:229: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
first = 'ParseResults contains params', second = '', msg = None
    def assert_in(first, second, msg=None):
        if msg is None:
>           assert first in second
E           AssertionError: assert 'ParseResults contains params' in ''
..\datalad\tests\utils_pytest.py:131: AssertionError
============================== warnings summary ===============================
datalad/support/tests/test_cookies.py::test_no_blows

will file an issue

yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Aug 3, 2022
… URL on windows

Since paths are actually containg \ not / like on POSIX/in urls.

Prehistory:

In datalad#6891 I have missed the fact that windows on appveyor was not happy! (on the importance of having CI green!!!!):

https://ci.appveyor.com/project/mih/datalad/builds/44287617/job/fgp764p65r1ba5oj

	____________________________ test_pathri_guessing _____________________________
	filename = 'C:\\DLTMP\\datalad_temp_pwl4puuu'
		@with_tempfile
		def test_pathri_guessing(filename=None):
			# Complaining about ;param only at DEBUG level
			# see datalad#6872
			with swallow_logs(new_level=logging.DEBUG) as cml:
				# we don't "care" about params ATM so there is a warning if there are any
				ri = RI(f"{filename};param")
				assert isinstance(ri, PathRI)
	>           assert_in('ParseResults contains params', cml.out)
	..\datalad\support\tests\test_network.py:229:
	_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
	first = 'ParseResults contains params', second = '', msg = None
		def assert_in(first, second, msg=None):
			if msg is None:
	>           assert first in second
	E           AssertionError: assert 'ParseResults contains params' in ''
	..\datalad\tests\utils_pytest.py:131: AssertionError
	============================== warnings summary ===============================
	datalad/support/tests/test_cookies.py::test_no_blows

and indeed only on windows:

	(git)smaug:/mnt/datasets/datalad/ci/logs/2022/08[master]git
	$> git grep FAIL.*test_pathri_guessing
	02/pr/6907/7226824/appveyor-8314-failed/84vlamobs20bbih0.txt:[00:46:09] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	02/push/rf-minor/7226824/appveyor-8313-failed/pl2n7yc5jfgv9lpe.txt:[00:40:49] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...

	$> cd ../07

	$> git grep FAIL.*test_pathri_guessing
	26/pr/6891/2dfa4f6/appveyor-8280-failed/fgp764p65r1ba5oj.txt:[00:39:18] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	29/pr/6898/fe417e0/appveyor-8300-failed/twx5hrhmt2u5ks4n.txt:[00:40:33] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	29/pr/6899/c3e2ed8/appveyor-8301-failed/8cbph0v7xetnpm8g.txt:[00:39:12] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	29/pr/6902/2f9e1ac/appveyor-8302-failed/l4xy5rv7a0p88wk5.txt:[00:39:36] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	29/push/maint/825c8e0/appveyor-8299-failed/xn68pxgkuxf27iya.txt:[00:37:40] FAILED ..\datalad\support\tests\test_network.py::test_pathri_guessing - Asser...
	git grep FAIL.*test_pathri_guessing  5.46s user 8.23s system 102% cpu 13.398 total
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants