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

Enable currently-ignored linting rules E721, E722, F403 #573

Merged
merged 2 commits into from
Aug 19, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion icepyx/quest/dataset_scripts/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
from .dataset import *
from .dataset import * # noqa: F403
2 changes: 1 addition & 1 deletion icepyx/quest/dataset_scripts/argo.py
Original file line number Diff line number Diff line change
@@ -477,7 +477,7 @@
profile_data = self._download_profile(i)
profile_df = self._parse_into_df(profile_data[0])
merged_df = pd.concat([merged_df, profile_df], sort=False)
except:
except Exception:

Check warning on line 480 in icepyx/quest/dataset_scripts/argo.py

Codecov / codecov/patch

icepyx/quest/dataset_scripts/argo.py#L480

Added line #L480 was not covered by tests
print("\tError processing profile {0}. Skipping.".format(i))

# now that we have a df from this round of downloads, we can add it to any existing dataframe
4 changes: 2 additions & 2 deletions icepyx/quest/quest.py
Original file line number Diff line number Diff line change
@@ -187,7 +187,7 @@
except KeyError:
v.search_data()

except:
except Exception:

Check warning on line 190 in icepyx/quest/quest.py

Codecov / codecov/patch

icepyx/quest/quest.py#L190

Added line #L190 was not covered by tests
dataset_name = type(v).__name__
print("Error querying data from {0}".format(dataset_name))

@@ -226,7 +226,7 @@
except KeyError:
msg = v.download()
print(msg)
except:
except Exception:

Check warning on line 229 in icepyx/quest/quest.py

Codecov / codecov/patch

icepyx/quest/quest.py#L229

Added line #L229 was not covered by tests
dataset_name = type(v).__name__
print("Error downloading data from {0}".format(dataset_name))

2 changes: 1 addition & 1 deletion icepyx/tests/test_Earthdata.py
Original file line number Diff line number Diff line change
@@ -65,7 +65,7 @@ def earthdata_login(uid=None, pwd=None, email=None, s3token=False) -> bool:
try:
url = "urs.earthdata.nasa.gov"
mock_uid, _, mock_pwd = netrc.netrc(netrc).authenticators(url)
except:
except Exception:
mock_uid = os.environ.get("EARTHDATA_USERNAME")
mock_pwd = os.environ.get("EARTHDATA_PASSWORD")

8 changes: 4 additions & 4 deletions icepyx/tests/test_quest.py
Original file line number Diff line number Diff line change
@@ -26,9 +26,9 @@ def test_add_is2(quest_instance):

obs = quest_instance.datasets

assert type(obs) == dict
assert isinstance(obs, dict)
assert exp_key in obs.keys()
assert type(obs[exp_key]) == exp_type
assert isinstance(obs[exp_key], exp_type)
assert quest_instance.datasets[exp_key].product == prod


@@ -40,9 +40,9 @@ def test_add_argo(quest_instance):

obs = quest_instance.datasets

assert type(obs) == dict
assert isinstance(obs, dict)
assert exp_key in obs.keys()
assert type(obs[exp_key]) == exp_type
assert isinstance(obs[exp_key], exp_type)
Copy link
Member

Choose a reason for hiding this comment

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

Note: this is a test so we have automatic confirmation it's fine, but for anyone not aware, there is a difference in behavior between type(...) == ... and isintance(..., ...) - isinstance evaluates True with inheritance:

>>> class Foo(str):
...     ...
... 
>>> type(Foo()) is str
False
>>> type(Foo()) is Foo
True
>>> isinstance(Foo(), str)
True
>>> isinstance(Foo(), Foo)
True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. The == --> is transition would be good enough to silence this ruff rule. The "comparing types" thing is frowned on in PEP8 and there is a different ruff rule that covers that.

Copy link
Member

@mfisher87 mfisher87 Aug 19, 2024

Choose a reason for hiding this comment

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

The more I think about it, the more I think we should go back to exact type comparison with type(...) is .... Since this is a test, loosening the comparison enables the underlying code to change more without breaking the test. We an always come back to this. My little mind is just thinking about the "foolish consistency" part of PEP8.

Would you mind?

assert set(quest_instance.datasets[exp_key].params) == set(params)


12 changes: 2 additions & 10 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -62,6 +62,8 @@ version_file_template = 'version = "{version}"'
local_scheme = "node-and-date"
fallback_version = "unknown"

[tool.codespell]
ignore-words-list = "aas,socio-economic,toi"

[tool.ruff]
# DevGoal: Lint and format all Jupyter Notebooks, remove below.
@@ -71,9 +73,6 @@ extend-exclude = ["*.ipynb"]
# docstring-code-format = true
# docstring-code-line-length = "dynamic"

[tool.codespell]
ignore-words-list = "aas,socio-economic,toi"

[tool.ruff.lint]
select = [
"E", # pycodestyle
@@ -87,13 +86,6 @@ ignore = [
# overlong comments.
# See: https://github.com/psf/black/issues/1713#issuecomment-1357045092
"E501",
# TODO: remove ignores below this line
# comparison syntax in tests
"E721",
# bare except
"E722",
# unable to detect undefined names
"F403",
]

[tool.ruff.lint.per-file-ignores]
Loading