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,