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

Make sydent.terms pass mypy --strict #428

Merged
merged 4 commits into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions changelog.d/428.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make `sydent.terms` pass `mypy --strict`.
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ files = [
# find sydent tests -type d -not -name __pycache__ -exec bash -c "mypy --strict '{}' > /dev/null" \; -print
"sydent/config",
"sydent/db",
"sydent/terms",
"sydent/threepid",
"sydent/users",
"sydent/util",
Expand Down
72 changes: 46 additions & 26 deletions sydent/terms/terms.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,37 @@
# limitations under the License.

import logging
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set
from typing import TYPE_CHECKING, Dict, List, Mapping, Optional, Set, Union

import yaml
from typing_extensions import TypedDict

if TYPE_CHECKING:
from sydent.sydent import Sydent

logger = logging.getLogger(__name__)


class TermConfig(TypedDict):
master_version: str
docs: Mapping[str, "Policy"]


class Policy(TypedDict):
version: str
langs: Mapping[str, "LocalisedPolicy"]


class LocalisedPolicy(TypedDict):
name: str
url: str


VersionOrLang = Union[str, LocalisedPolicy]


class Terms:
def __init__(self, yamlObj: Optional[Dict[str, Any]]) -> None:
def __init__(self, yamlObj: Optional[TermConfig]) -> None:
"""
:param yamlObj: The parsed YAML.
"""
Expand All @@ -35,20 +54,19 @@ def getMasterVersion(self) -> Optional[str]:
:return: The global (master) version of the terms, or None if there
are no terms of service for this server.
"""
version = None if self._rawTerms is None else self._rawTerms["master_version"]

# Ensure we're dealing with unicode.
if version and isinstance(version, bytes):
version = version.decode("UTF-8")
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why its safe to remove the bytes check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • version comes from self._rawTerms
  • self._rawTerms is a dict, set in the constructor. I can't see anywhere that it's mutated.
  • It'll be set to either None or yaml.safe_load(...)
  • AFAIK the latter can't produce a bytes object....

But that last bullet isn't correct. There's a !!binary "language independent scalar type" in YAML. So I guess it's possible that this could be a bytes object.

I am incredibly tempted to just pull in jsonschema and use that to enforce the config shape and type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but I've refrained and checked the string type after we load the yaml.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, that works. In general the flow is a bit weird here as we validate the data, then pass around the dict and then pull out data from it, which means its quite hard to see what has been validated and what hasn't. But let's not fix that up in a typing PR.


return version

def getForClient(self) -> Dict[str, dict]:
if self._rawTerms is None:
return None
return self._rawTerms["master_version"]

def getForClient(self) -> Dict[str, Dict[str, Dict[str, VersionOrLang]]]:
# Examples:
# "policy" -> "terms_of_service", "version" -> "1.2.3"
# "policy" -> "terms_of_service", "en" -> LocalisedPolicy
"""
:return: A dict which value for the "policies" key is a dict which contains the
"docs" part of the terms' YAML. That nested dict is empty if no terms.
"""
policies = {}
policies: Dict[str, Dict[str, VersionOrLang]] = {}
if self._rawTerms is not None:
for docName, doc in self._rawTerms["docs"].items():
policies[docName] = {
Expand All @@ -66,11 +84,6 @@ def getUrlSet(self) -> Set[str]:
for docName, doc in self._rawTerms["docs"].items():
for langName, lang in doc["langs"].items():
url = lang["url"]

# Ensure we're dealing with unicode.
if url and isinstance(url, bytes):
url = url.decode("UTF-8")

urls.add(url)
return urls

Expand All @@ -87,36 +100,43 @@ def urlListIsSufficient(self, urls: List[str]) -> bool:
agreed = set()
urlset = set(urls)

if self._rawTerms is not None:
if self._rawTerms is None:
if urls:
raise ValueError("No configured terms, but user accepted some terms")
else:
return True

else:
for docName, doc in self._rawTerms["docs"].items():
for lang in doc["langs"].values():
if lang["url"] in urlset:
agreed.add(docName)
break

required = set(self._rawTerms["docs"].keys())
return agreed == required
required = set(self._rawTerms["docs"].keys())
return agreed == required


def get_terms(sydent: "Sydent") -> Optional[Terms]:
"""Read and parse terms as specified in the config.

:returns Terms
"""
"""Read and parse terms as specified in the config."""
# TODO - move some of this to parse_config

termsPath = sydent.config.general.terms_path

try:
termsYaml = None

if termsPath == "":
return Terms(None)

with open(termsPath) as fp:
termsYaml = yaml.safe_load(fp)

# TODO use something like jsonschema instead of this handwritten code.
if "master_version" not in termsYaml:
raise Exception("No master version")
elif not isinstance(termsYaml["master_version"], str):
raise TypeError(
f"master_version should be a string, not {termsYaml['master_version']!r}"
)
if "docs" not in termsYaml:
raise Exception("No 'docs' key in terms")
for docName, doc in termsYaml["docs"].items():
Expand Down