From 30791da9fd02921fc42ad3605c1c9bf8566d9e78 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Mon, 9 Nov 2026 12:09:33 -0800 Subject: [PATCH] fix(salary): split combined name-and-city workbook columns (#660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(salary): split combined name-and-city workbook columns The IDA demonstration workbook heads its company column "Firmanavn og by" and stores "Ørsted A/S, Fredericia" in that one cell. After the header is detected, convert_salary_excel copied the whole cell into company with city "". salary_lookup --city only reads entry["city"], so the documented lookup for Ørsted in Fredericia missed. When the company header also names a city and there is no city column, split the cell on the last ", ". A Company-only header still keeps commas in the name. * fix(salary): split a combined name-and-city cell on the last comma Real IDA cells have no space after the separating comma ("Ørsted A/S,Fredericia"), and some company names contain ", " themselves ("Rambøll Danmark A/S, Odense,Odense C"). Splitting on the last ", " missed almost every real row, so --city still dropped out. split_combined_company_city now splits on the last "," and strips both parts. Test fixtures use the real unspaced cell shape, and a new unit test covers a company name that contains a comma. The docstrings, CHANGELOG entry, tools/README_SALARY_TOOL.md and the README salary section describe the new split. * docs(changelog): move the combined-city entry under Unreleased The merge of master brought in the 1.7.3 release section, which left this PR's Fixed entry under an already released version. Move it to the Unreleased Fixed list. * chore: drop the CHANGELOG hunk, the file is frozen since #800 Release notes are generated from PR titles now. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Mads Lorentzen Co-authored-by: Claude Fable 5.1 --- README.md | 2 + tests/test_convert_salary_excel.py | 51 +++++++++++++++- tests/test_salary_combined_city_cli.py | 80 ++++++++++++++++++++++++++ tools/README_SALARY_TOOL.md | 1 + tools/convert_salary_excel.py | 34 ++++++++++- 5 files changed, 165 insertions(+), 3 deletions(-) create mode 100644 tests/test_salary_combined_city_cli.py diff --git a/README.md b/README.md index 98b9a2c4..b819445f 100644 --- a/README.md +++ b/README.md @@ -318,6 +318,8 @@ Market-specific *data sources* (a national salary database, local award-rate tab The salary tool works with any salary data you provide (union statistics, Glassdoor exports, personal research, etc.). See `tools/README_SALARY_TOOL.md` for the expected format and setup. If you don't have salary data, the salary step is simply skipped. +`tools/convert_salary_excel.py` turns an Excel export into that format. When the company column also holds the city, as in the IDA workbook's "Firmanavn og by" column (`Ørsted A/S,Fredericia`), the converter splits each cell on its last comma and strips both parts, so `salary_lookup.py --city` can filter. A company name that itself contains ", " stays intact. + ### Starting over To wipe your profile data and start fresh: diff --git a/tests/test_convert_salary_excel.py b/tests/test_convert_salary_excel.py index 7fb653fb..5af0f450 100644 --- a/tests/test_convert_salary_excel.py +++ b/tests/test_convert_salary_excel.py @@ -173,13 +173,60 @@ class DetectColumnTypeTests(unittest.TestCase): self.assertTrue(header_matches("Firmanavn og by", COMPANY_PATTERNS)) ws = FakeWorksheet([ ("Firmanavn og by", "Antal", "Indeks"), - ("Example A/S, Aarhus", 12, 105.0), + ("Example A/S,Aarhus", 12, 105.0), ]) companies = parse_sheet(ws) self.assertEqual(len(companies), 1) - self.assertEqual(companies[0]["company"], "Example A/S, Aarhus") + self.assertEqual(companies[0]["company"], "Example A/S") + self.assertEqual(companies[0]["city"], "Aarhus") self.assertEqual(companies[0]["categories"]["all_employees"], {"count": 12, "index": 105.0}) + def test_parse_sheet_does_not_split_a_comma_in_a_company_only_column(self): + # A "Company" header is not a combined name-and-city column. "Acme, Inc." + # stays the company; splitting it would invent a city named "Inc.". + ws = FakeWorksheet([ + ("Company", "Antal", "Indeks"), + ("Acme, Inc.", 12, 105.0), + ]) + companies = parse_sheet(ws) + self.assertEqual(len(companies), 1) + self.assertEqual(companies[0]["company"], "Acme, Inc.") + self.assertEqual(companies[0]["city"], "") + + def test_parse_sheet_splits_english_combined_company_and_city_header(self): + ws = FakeWorksheet([ + ("Company and City", "Count", "Index"), + ("Ørsted A/S,Kongens Lyngby", 200, 105.2), + ]) + companies = parse_sheet(ws) + self.assertEqual(len(companies), 1) + self.assertEqual(companies[0]["company"], "Ørsted A/S") + self.assertEqual(companies[0]["city"], "Kongens Lyngby") + + def test_parse_sheet_splits_on_the_last_comma_when_the_company_contains_one(self): + # IDA cells have no space after the separating comma. A company can + # itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C"). + ws = FakeWorksheet([ + ("Firmanavn og by", "Antal", "Indeks"), + ("Rambøll Danmark A/S, Odense,Odense C", 12, 105.0), + ("Ørsted A/S,Fredericia", 200, 105.2), + ]) + companies = parse_sheet(ws) + self.assertEqual( + [(c["company"], c["city"]) for c in companies], + [("Rambøll Danmark A/S, Odense", "Odense C"), ("Ørsted A/S", "Fredericia")], + ) + + def test_parse_sheet_keeps_a_combined_cell_with_no_comma_as_company_only(self): + ws = FakeWorksheet([ + ("Firmanavn og by", "Antal", "Indeks"), + ("Example A/S", 12, 105.0), + ]) + companies = parse_sheet(ws) + self.assertEqual(len(companies), 1) + self.assertEqual(companies[0]["company"], "Example A/S") + self.assertEqual(companies[0]["city"], "") + def test_parse_sheet_detects_city_column_with_token_header(self): # City headers are matched with the same token-based header_matches() # used for the company column, not exact string equality. Real-world diff --git a/tests/test_salary_combined_city_cli.py b/tests/test_salary_combined_city_cli.py new file mode 100644 index 00000000..0bb3b94c --- /dev/null +++ b/tests/test_salary_combined_city_cli.py @@ -0,0 +1,80 @@ +"""A combined name-and-city workbook column must feed salary_lookup --city. + +The IDA demonstration workbook heads its company column "Firmanavn og by" +and stores "Ørsted A/S,Fredericia" in that one cell (no space after the +comma). convert_salary_excel used to copy the whole cell into company with +city "". The documented +lookup `python3 salary_lookup.py "Ørsted" --city "Fredericia"` then missed, +even though the city was in the converted file - just not in the field +--city reads. +""" +import json +import shutil +import subprocess +import sys +import unittest +from pathlib import Path +from tempfile import TemporaryDirectory + +try: + import openpyxl +except ImportError: + openpyxl = None + +REPO = Path(__file__).resolve().parent.parent + + +@unittest.skipUnless(openpyxl, "requires optional openpyxl (installed in CI)") +class CombinedCityWorkbookCLI(unittest.TestCase): + def convert_and_lookup(self, headers, row, *lookup_args): + with TemporaryDirectory() as directory: + root = Path(directory) + workbook = root / "salary.xlsx" + output = root / "salary_data.json" + book = openpyxl.Workbook() + try: + book.active.append(headers) + book.active.append(row) + book.save(workbook) + finally: + book.close() + conversion = subprocess.run( + [sys.executable, str(REPO / "tools/convert_salary_excel.py"), + str(workbook), "--output", str(output)], + capture_output=True, text=True, encoding="utf-8", + ) + self.assertEqual(conversion.returncode, 0, conversion.stderr) + companies = json.loads(output.read_text(encoding="utf-8"))["companies"] + shutil.copyfile(REPO / "salary_lookup.py", root / "salary_lookup.py") + lookup = subprocess.run( + [sys.executable, str(root / "salary_lookup.py"), *lookup_args], + capture_output=True, text=True, encoding="utf-8", + ) + return companies, lookup + + def test_ida_firmanavn_og_by_row_is_findable_with_city_filter(self): + companies, lookup = self.convert_and_lookup( + ["Firmanavn og by", "Antal", "Indeks"], + ["Ørsted A/S,Fredericia", 200, 105.2], + "Ørsted", "--city", "Fredericia", + ) + self.assertEqual(companies, [{ + "company": "Ørsted A/S", + "city": "Fredericia", + "categories": {"all_employees": {"count": 200, "index": 105.2}}, + }]) + self.assertEqual(lookup.returncode, 0, lookup.stdout + lookup.stderr) + self.assertIn("Ørsted A/S", lookup.stdout) + self.assertIn("Fredericia", lookup.stdout) + self.assertNotIn("No results found", lookup.stdout) + + def test_separate_city_column_is_unchanged(self): + companies, lookup = self.convert_and_lookup( + ["Company", "City", "Count", "Index"], + ["Novo Nordisk A/S", "Bagsværd", 500, 108.5], + "Novo Nordisk", "--city", "Bagsværd", + ) + self.assertEqual(companies[0]["company"], "Novo Nordisk A/S") + self.assertEqual(companies[0]["city"], "Bagsværd") + self.assertEqual(lookup.returncode, 0, lookup.stdout + lookup.stderr) + self.assertIn("Novo Nordisk A/S", lookup.stdout) diff --git a/tools/README_SALARY_TOOL.md b/tools/README_SALARY_TOOL.md index 68826385..f2dd1b42 100644 --- a/tools/README_SALARY_TOOL.md +++ b/tools/README_SALARY_TOOL.md @@ -79,6 +79,7 @@ On Windows, use `py` if that is how Python is exposed on your PATH. If your syst The converter auto-detects the Excel layout: - Looks for a "Company"/"Firma" column and an optional "City"/"By" column +- A combined header such as "Firmanavn og by" or "Company and City" (no separate city column) is split on the last comma, with both parts stripped, so `salary_lookup.py --city` can filter. Cells look like "Ørsted A/S,Fredericia"; a company name may itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C") - Treats remaining columns as salary data (auto-pairs count/index columns) ### Option C: Build from research diff --git a/tools/convert_salary_excel.py b/tools/convert_salary_excel.py index 85d1c7c8..4fb445f2 100644 --- a/tools/convert_salary_excel.py +++ b/tools/convert_salary_excel.py @@ -18,7 +18,9 @@ The output file (salary_data.json) will be written to the repository root. Expected Excel format: - A header row with column names - A "Company" or "Firma" column (required) - - An optional "City" or "By" column + - An optional "City" or "By" column. A combined header such as + "Firmanavn og by" / "Company and City" (no separate city column) + is split on the last comma, and both parts are stripped, into company and city. - Any number of numeric data columns (salary index, count, etc.) The script auto-detects the header row and column layout. For Excel files @@ -97,6 +99,28 @@ def parse_numeric_cell(value): return _finite_float(text) +def split_combined_company_city(value): + """Split a combined name-and-city cell on its last comma. + + The IDA demonstration workbook heads its company column "Firmanavn og by" + and stores "Ørsted A/S,Fredericia" (no space after the comma). salary_lookup's + --city filter only reads entry["city"], so leaving the city in the company + string makes the documented + `python3 salary_lookup.py "Ørsted" --city "Fredericia"` miss. Split on the + last comma and strip both parts: a city may contain spaces ("Kongens Lyngby") + and a company may contain ", " ("Rambøll Danmark A/S, Odense,Odense C" + keeps "Rambøll Danmark A/S, Odense"). A cell with no comma stays the company + with an empty city. + """ + if not isinstance(value, str) or "," not in value: + return value, "" + company, city = value.rsplit(",", 1) + company, city = company.strip(), city.strip() + if company and city: + return company, city + return value, "" + + def header_matches(header, patterns): """Return True when a header contains a meaningful pattern match. @@ -224,6 +248,12 @@ def parse_sheet(ws, sheet_label=None): print(f"Warning: Could not find company column in sheet '{ws.title}'.", file=sys.stderr) return [] + # "Firmanavn og by" / "Company and City": one column holds both, and there + # is no separate city column. Split the cell so --city can see the city. + combined_name_and_city = city_col is None and header_matches( + headers[company_col], CITY_PATTERNS + ) + # Identify data columns (everything that's not company/city or an identifier) data_cols = [] for i, h in enumerate(headers): @@ -327,6 +357,8 @@ def parse_sheet(ws, sheet_label=None): city_name = str(row[city_col]).strip() else: city_name = "" + if combined_name_and_city and not city_name: + company_name, city_name = split_combined_company_city(company_name) entry = { "company": company_name,