style(black): format acquisition with `black`, line-length=100 #1189

dshemetov · 2023-06-05T22:17:30Z

A version of #1186 with black's line-length=100. Recommend using GitHub's ignore whitespace feature when looking at diffs.

TL;DR: it mostly looks good. The main jank is, as Katie put it below, "repeated calls should use the same wrap mode".

Add .pylintrc configs from covidcast-indicators. Don't enforce pylint, just keep the same configs.
Apply pyupgrade for a few minor changes.
Use flynt to automatically convert many of the % and .format strings into official f-strings. It was pretty straightforward pip install flynt and flynt src/acquisition -a -l 999 --verbose (-a is aggressive, it won't change % strings otherwise, -l 999 is line-length, it won't change long strings otherwise, --verbose is to have it log its changes). With this, it had a 94/98 success rate on %-style expressions, so I had to go fix a few manually.
Write the correct commits into ignore blame refs file, once this PR is finalized

src/acquisition/afhsb/afhsb_csv.py

src/acquisition/covid_hosp/common/database.py

dshemetov · 2023-06-05T22:28:15Z

src/acquisition/covid_hosp/common/database.py

+        sql = (
+            f"INSERT INTO `{self.table_name}` (`id`, `{self.publication_col_name}`, {columns}) "
+            f"VALUES ({value_placeholders})"
+        )


more vertical space waste

disagree; in this case highlighting INSERT INTO and VALUES improves scan-ability

separating those two was already in the original code though. black added the parentheses, doubling the the vertical space of this code :/

src/acquisition/covid_hosp/facility/database.py

src/acquisition/covid_hosp/state_daily/database.py

src/acquisition/covid_hosp/state_timeseries/database.py

src/acquisition/ght/ght_update.py

src/acquisition/norostat/norostat_add_history.py

nmdefries

I like this one a lot better than with line-length=200. The formatting is acceptable overall and the wrapping style is more readable.

It's not 100% ideal in several places, but there are too many changes here to futz around with manually fixing and freezing more than a few individual lines.

I'd say the most worthwhile use of time would be deciding how we want to format the Columndef("previous_day_admission_adult_covid_confirmed_20-29_7_day_sum", "previous_day_admission_adult_covid_confirmed_20_29_7_day_sum", int), lines in all the database.py variants, and call it good after that.

nmdefries · 2023-06-07T21:21:38Z

src/acquisition/covid_hosp/common/utils.py

+        if logger and len(dfs) > 7:
+            logger.warning(
+                "expensive operation",
+                msg="concatenating more than 7 files may result in long running times",
+                count=len(dfs),


better in this version

nmdefries · 2023-06-07T21:23:47Z

src/acquisition/fluview/fluview_update.py

+        "REGION TYPE",
+        "REGION",
+        "YEAR",
+        "WEEK",
+        "TOTAL SPECIMENS",
+        "TOTAL A",
+        "TOTAL B",
+        "PERCENT POSITIVE",
+        "PERCENT A",
+        "PERCENT B",
+    ]:


Better wrapping here

? I don't notice a change

Sorry for the ambiguity, I was comparing to the line-length: 200 behavior here. Some other comments in this review are also discussing changes compared to the length-200 version, rather than the before-and-after here.

nmdefries · 2023-06-07T21:35:34Z

src/acquisition/ecdc/ecdc_db_update.py

-        '--test',
-        action='store_true',
-        help='do dry run only, do not update the database'
+        "--test", action="store_true", help="do dry run only, do not update the database"


same with the vertical space comment

nmdefries · 2023-06-07T21:38:19Z

src/acquisition/fluview/fluview_update.py

+        args = [
+            row["total_specimens"],
+            row["total_a"],
+            row["total_b"],
+            row["percent_positive"],
+            row["percent_a"],
+            row["percent_b"],
+        ]


Maybe not the best use of space but acceptable

i prefer it

src/acquisition/twtr/healthtweets.py

dshemetov · 2023-06-07T23:40:48Z

Absolutely agree with your review comment.

Formatting ColumnDef lines is actually moot, since we have a refactor that removes those lines already.

So I'm just going to exclude covid_hosp from this PR, to avoid conflicts with that PR. We can format that code after those refactors.

krivard

Places it falls over:

repeated calls should use the same wrap mode
same-line comments on too-long lines should be moved to the previous line before applying wrapping rules

otherwise seems pretty good!

krivard · 2023-06-21T15:07:50Z

src/acquisition/cdcp/cdc_dropbox_receiver.py

+    zip_path = f"{DELPHI_BASE_DIR}/dropbox_{timestamp}.zip"
+    print(f"downloading into delphi:{zip_path}")
+    with ZipFile(zip_path, "w", ZIP_DEFLATED) as zf:
+        for name in save_list:


this is when I wish github let you provide manual alignment markers to make diffs make sense

if you do "hide whitespace" up in the gear above, it makes these diffs way more readable

src/acquisition/cdcp/cdc_extract.py

src/acquisition/covid_hosp/common/database.py

krivard · 2023-06-21T15:19:38Z

src/acquisition/covid_hosp/common/database.py

+        sql = (
+            f"INSERT INTO `{self.table_name}` (`id`, `{self.publication_col_name}`, {columns}) "
+            f"VALUES ({value_placeholders})"
+        )


disagree; in this case highlighting INSERT INTO and VALUES improves scan-ability

krivard · 2023-06-21T15:27:15Z

src/acquisition/fluview/fluview_update.py

+        "--test", action="store_true", help="do dry run only, do not update the database"
+    )
+    parser.add_argument(
+        "--file", type=str, help="load an existing zip file (otherwise fetch current data)"
+    )
+    parser.add_argument(
+        "--issue", type=int, help="issue of the file (e.g. 201740); used iff --file is given"


hmmmm I'm skeptical about these. if it were just one, fine, but with three of them it's helpful to have whitespace highlight argument alignment

src/acquisition/twtr/healthtweets.py

krivard · 2023-06-21T15:32:02Z

src/acquisition/fluview/fluview_update.py

+        "REGION TYPE",
+        "REGION",
+        "YEAR",
+        "WEEK",
+        "TOTAL SPECIMENS",
+        "TOTAL A",
+        "TOTAL B",
+        "PERCENT POSITIVE",
+        "PERCENT A",
+        "PERCENT B",
+    ]:


? I don't notice a change

src/acquisition/norostat/norostat_add_history.py

src/acquisition/ght/ght_update.py

src/acquisition/afhsb/afhsb_csv.py

dshemetov · 2023-06-21T21:50:18Z

Trying to test and fix the regex in question that SonarCloud doesn't like, but the norostat code doesn't even seem to be run correctly. It breaks in line 74 of that same file, where the output of html_root.xpath(...) is just [] for me 🤷 @krivard this makes me think this source doesn't run. Is it worth fixing?

krivard · 2023-06-22T18:59:57Z

this makes me think this source doesn't run. Is it worth fixing?

we're still running a norostat cronicle job daily, but it's been failing since November 2020. Checking with R&R whether to keep it or not.

krivard · 2023-06-23T18:01:49Z

We can kill norostat acquisition

dshemetov · 2023-06-23T18:42:01Z

Dropped Norostat changes from this and changed the base to the no-Norostat PR

Co-authored-by: Katie Mazaitis <[email protected]>

sonarcloud · 2023-06-26T17:43:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
42 Code Smells

No Coverage information
2.3% Duplication

krivard

🚀

thanks for fixing all the fmt: on|off cases!

nmdefries

Looks great!

dshemetov requested a review from nmdefries June 5, 2023 22:17

dshemetov mentioned this pull request Jun 5, 2023

style(black): format acquisition with black, line-length=200 #1186

Closed

4 tasks

dshemetov commented Jun 5, 2023

View reviewed changes

src/acquisition/afhsb/afhsb_csv.py Outdated Show resolved Hide resolved

dshemetov commented Jun 5, 2023

View reviewed changes

src/acquisition/afhsb/afhsb_csv.py Outdated Show resolved Hide resolved

dshemetov force-pushed the ds/format100 branch from 1579b1f to 658e1d4 Compare June 5, 2023 23:38

dshemetov commented Jun 5, 2023

View reviewed changes

nmdefries reviewed Jun 7, 2023

View reviewed changes

krivard reviewed Jun 21, 2023

View reviewed changes

dshemetov marked this pull request as ready for review June 21, 2023 20:33

dshemetov force-pushed the ds/format100 branch from b6eb879 to 6471c01 Compare June 21, 2023 21:10

dshemetov changed the title ~~[test] try black line-length=100~~ style(black): format acquisition with black, line-length=100 Jun 21, 2023

krivard mentioned this pull request Jun 23, 2023

Deactivate & mothball norostat #1205

Closed

4 tasks

dshemetov force-pushed the ds/format100 branch from dee5e05 to 2795101 Compare June 23, 2023 18:41

dshemetov changed the base branch from dev to ds/del-norostat-acquisition June 23, 2023 18:41

Base automatically changed from ds/del-norostat-acquisition to dev June 23, 2023 19:51

dshemetov added 10 commits June 26, 2023 09:51

ci(black): set line-length 100

60cdbb1

style(black): format cdc acquisition

980b0b7

style(black): format covidcast_nowcast acquisition

9e6ff16

style(black): format ecdc acquisition

d1141d9

style(black): format flusurv acquisition

08af0f6

style(black): format fluview acquisition

0133ef2

style(black): format ght acquisition

b8900a0

style(black): format kcdc acquisition

a849384

style(black): format nidss acquisition

d04af3c

style(black): format paho acquisition

7f60fbb

dshemetov and others added 11 commits June 26, 2023 09:51

style(black): format quidel acquisition

b9ceb40

style(black): format twitter acquisition

07ed83e

style(black): format wiki acquisition

923852e

ci(sonar): tempfiles for security warnings

c827e54

style: add .editorconfig

76ddfbf

style(pylint): add pylint config

145dd42

Update src/acquisition/cdcp/cdc_extract.py

1e7319e

Co-authored-by: Katie Mazaitis <[email protected]>

style(black): add fmt off tags around parser.add_argument calls

b00f11b

style: update .editorconfig

dd1b089

style(flynt): convert .format and % strings to f-strings

7a27a3a

gh: add .git-blame-ignore-revs

8459cc2

dshemetov force-pushed the ds/format100 branch from 2795101 to 8459cc2 Compare June 26, 2023 16:58

style(acquisition): minor formatting fixes

f93f020

dshemetov requested review from krivard and nmdefries June 26, 2023 19:44

krivard approved these changes Jun 26, 2023

View reviewed changes

dshemetov mentioned this pull request Jun 26, 2023

Design and enforce linting standards #962

Open

nmdefries approved these changes Jun 26, 2023

View reviewed changes

dshemetov merged commit 0609700 into dev Jun 26, 2023

dshemetov deleted the ds/format100 branch June 26, 2023 20:12

nolangormley mentioned this pull request Jun 27, 2023

Release Delphi Epidata 4.1.4 #1216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(black): format acquisition with `black`, line-length=100 #1189

style(black): format acquisition with `black`, line-length=100 #1189

dshemetov commented Jun 5, 2023 •

edited

Loading

dshemetov Jun 5, 2023

krivard Jun 21, 2023

dshemetov Jun 21, 2023

nmdefries left a comment •

edited

Loading

nmdefries Jun 7, 2023 •

edited

Loading

nmdefries Jun 7, 2023

krivard Jun 21, 2023

nmdefries Jun 21, 2023 •

edited

Loading

nmdefries Jun 7, 2023

nmdefries Jun 7, 2023

krivard Jun 21, 2023

dshemetov commented Jun 7, 2023 •

edited

Loading

krivard left a comment

krivard Jun 21, 2023

dshemetov Jun 26, 2023

krivard Jun 21, 2023

krivard Jun 21, 2023

krivard Jun 21, 2023

dshemetov commented Jun 21, 2023

krivard commented Jun 22, 2023

krivard commented Jun 23, 2023

dshemetov commented Jun 23, 2023

sonarcloud bot commented Jun 26, 2023

krivard left a comment

nmdefries left a comment

style(black): format acquisition with black, line-length=100 #1189

style(black): format acquisition with black, line-length=100 #1189

Conversation

dshemetov commented Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmdefries left a comment • edited Loading

Choose a reason for hiding this comment

nmdefries Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmdefries Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshemetov commented Jun 7, 2023 • edited Loading

krivard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshemetov commented Jun 21, 2023

krivard commented Jun 22, 2023

krivard commented Jun 23, 2023

dshemetov commented Jun 23, 2023

sonarcloud bot commented Jun 26, 2023

krivard left a comment

Choose a reason for hiding this comment

nmdefries left a comment

Choose a reason for hiding this comment

style(black): format acquisition with `black`, line-length=100 #1189

style(black): format acquisition with `black`, line-length=100 #1189

dshemetov commented Jun 5, 2023 •

edited

Loading

nmdefries left a comment •

edited

Loading

nmdefries Jun 7, 2023 •

edited

Loading

nmdefries Jun 21, 2023 •

edited

Loading

dshemetov commented Jun 7, 2023 •

edited

Loading