You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sebastianliebscher (via GitHub)" <gi...@apache.org> on 2023/05/10 10:07:34 UTC
[GitHub] [superset] sebastianliebscher opened a new pull request, #24001: chore: remove deprecated distutils
sebastianliebscher opened a new pull request, #24001:
URL: https://github.com/apache/superset/pull/24001
### SUMMARY
Distutils is deprecated. Followed the [migration advice](https://peps.python.org/pep-0632/#migration-advice).
- Replaces `distutils.version` with `packaging.version`
- Implements `distutils.util`s `strtobool` in `superset.utils` `core.py` with a unit test
### TESTING INSTRUCTIONS
`tox -e py38`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24001: chore: remove deprecated distutils
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24001:
URL: https://github.com/apache/superset/pull/24001#discussion_r1190804187
##########
tests/unit_tests/utils/test_core.py:
##########
@@ -84,3 +88,14 @@ def test_remove_extra_adhoc_filters(
) -> None:
remove_extra_adhoc_filters(original)
assert expected == original
+
+
+def test_parse_boolean_string():
+ true = ("y", "Y", "yes", "True", "t", "true", "True", "On", "on", "1")
+ false = ("n", "no", "f", "false", "off", "0", "Off", "No", "N", "foo")
+
+ for y in true:
+ assert parse_boolean_string(y)
+
+ for n in false:
Review Comment:
same here - let's try to avoid having two utils doing pretty much the same thing
##########
superset/utils/core.py:
##########
@@ -1787,8 +1787,25 @@ def indexed(
return idx
+def strtobool(val: str) -> bool:
+ """Convert a string representation of truth to true (1) or false (0).
+
+ True values are 'y', 'yes', 't', 'true', 'on', and '1'; false values
+ are 'n', 'no', 'f', 'false', 'off', and '0'. Raises ValueError if
+ 'val' is anything else.
+
+ Original implementation from deprecated distutils.
+ """
+ val = val.lower()
+ if val in {"y", "yes", "t", "true", "on", "1"}:
+ return True
+ if val in {"n", "no", "f", "false", "off", "0"}:
+ return False
+ raise ValueError("invalid truth value %r" % (val,))
Review Comment:
Just a thought: To simplify the code, would it be feasible to just place this logic inside `parse_boolean_string`? Or at least consolidate these somehow and just use that one util function everywhere..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24001: chore: remove deprecated distutils
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24001:
URL: https://github.com/apache/superset/pull/24001#discussion_r1191192689
##########
tests/unit_tests/utils/test_core.py:
##########
@@ -84,3 +90,26 @@ def test_remove_extra_adhoc_filters(
) -> None:
remove_extra_adhoc_filters(original)
assert expected == original
+
+
+def test_is_test():
+ orig_value = os.getenv("SUPERSET_TESTENV")
+
+ os.environ["SUPERSET_TESTENV"] = "true"
+ assert is_test()
+ os.environ["SUPERSET_TESTENV"] = "false"
+ assert not is_test()
+ os.environ["SUPERSET_TESTENV"] = ""
+ assert not is_test()
+
+ if orig_value is not None:
+ os.environ["SUPERSET_TESTENV"] = orig_value
+
+
+def test_parse_boolean_string():
+ true = ("y", "Y", "yes", "True", "t", "true", "On", "on", "1")
+ false = ("n", "N", "no", "False", "f", "false", "Off", "off", "0", "foo", "", None)
+ for val in true:
+ assert parse_boolean_string(val)
+ for val in false:
+ assert not parse_boolean_string(val)
Review Comment:
This is a perfect candidate for `pytest.mark.parametrize` could we refactor this test to that pattern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24001: chore: remove deprecated distutils
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24001:
URL: https://github.com/apache/superset/pull/24001#issuecomment-1541884135
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#24001](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (01c8ad1) into [master](https://app.codecov.io/gh/apache/superset/commit/48f3db946ed7ca46ebe175ead0afd1e607a113a2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (48f3db9) will **decrease** coverage by `11.23%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 01c8ad1 differs from pull request most recent head 384bb2d. Consider uploading reports for the commit 384bb2d to get more accurate results
```diff
@@ Coverage Diff @@
## master #24001 +/- ##
===========================================
- Coverage 68.20% 56.97% -11.23%
===========================================
Files 1941 1941
Lines 75287 75293 +6
Branches 8177 8177
===========================================
- Hits 51347 42898 -8449
- Misses 21845 30300 +8455
Partials 2095 2095
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `53.17% <33.33%> (-0.01%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.08% <33.33%> (-0.01%)` | :arrow_down: |
| python | `59.57% <100.00%> (-23.19%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.00% <91.66%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/elasticsearch.py](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `96.55% <100.00%> (ø)` | |
| [superset/db\_engine\_specs/presto.py](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `46.12% <100.00%> (-41.72%)` | :arrow_down: |
| [superset/utils/core.py](https://app.codecov.io/gh/apache/superset/pull/24001?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `65.68% <100.00%> (-25.24%)` | :arrow_down: |
... and [306 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24001/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #24001: chore: remove deprecated distutils
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #24001:
URL: https://github.com/apache/superset/pull/24001#discussion_r1190899058
##########
superset/utils/core.py:
##########
@@ -1787,8 +1787,25 @@ def indexed(
return idx
+def strtobool(val: str) -> bool:
+ """Convert a string representation of truth to true (1) or false (0).
+
+ True values are 'y', 'yes', 't', 'true', 'on', and '1'; false values
+ are 'n', 'no', 'f', 'false', 'off', and '0'. Raises ValueError if
+ 'val' is anything else.
+
+ Original implementation from deprecated distutils.
+ """
+ val = val.lower()
+ if val in {"y", "yes", "t", "true", "on", "1"}:
+ return True
+ if val in {"n", "no", "f", "false", "off", "0"}:
+ return False
+ raise ValueError("invalid truth value %r" % (val,))
Review Comment:
Thanks for your feedback! I consolidated `strtobool` and `parse_boolean_string`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #24001: chore: remove deprecated distutils
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #24001:
URL: https://github.com/apache/superset/pull/24001
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org