Skip to content
This repository has been archived by the owner on Jun 7, 2024. It is now read-only.

fix: tell type checkers that the config options are strings (trivial) #95

Closed

Conversation

tonyandrewmeyer
Copy link

@tonyandrewmeyer tonyandrewmeyer commented Apr 11, 2024

Applicable spec: N/A

Overview

ops<=2.12 wrongly has self.config[x] typed as str, when actually it could be an int, float, bool, or str, depending on the config type. We're fixing this in ops:#1183, but that will break static checking that currently assumes that the config is a str (because ops doesn't validate the schema, so all options will be bool|int|float|str).

This PR adds a typing.cast call where the config values are loaded and used where the type should be str.

Rationale

Covered in the overview.

Juju Events Changes

N/A

Module Changes

N/A

Library Changes

N/A

Checklist

No doc changes.

@tonyandrewmeyer tonyandrewmeyer requested a review from a team as a code owner April 11, 2024 05:26
@yanksyoon
Copy link

/canonical/self-hosted-runners/run-workflows 8554612

Copy link
Contributor

Test coverage for 8554612

Name           Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------
src/charm.py     188      3     68      5    97%   302->305, 396, 403->391, 409, 417
----------------------------------------------------------
TOTAL            188      3     68      5    97%

Static code analysis report

Run started:2024-04-11 06:26:19.315047

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1059
  Total lines skipped (#nosec): 0
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@arturo-seijas
Copy link
Collaborator

arturo-seijas commented May 3, 2024

Hi! I think I've done and merge the same change in #99. Thanks for the contribution in any case :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants