Disallow certain values for IDs

[alexdewar]

Description

We don't want users to name their proceses all or whatever as it'll break things, so let's prohibit it. I've currently prohibited empty strings, all and annual (could cause issues if a user names a season this).

Closes #548.

Type of change

• Bug fix (non-breaking change to fix an issue)
• New feature (non-breaking change to add functionality)
• Refactoring (non-breaking, non-functional change to improve maintainability)
• Optimization (non-breaking change to speed up the code)
• Breaking change (whatever its nature)
• Documentation (improve or add documentation)

Key checklist

• All tests pass: $ cargo test
• The documentation builds and looks OK: $ cargo doc

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[Copilot]

Pull Request Overview

This PR introduces a bug fix that disallows invalid ID values by prohibiting empty strings and the forbidden keywords "all" and "annual".

• Replaces the derived implementation of Deserialize with a custom one that enforces the ID restrictions.
• Adds trimming and forbidden value checks during deserialization.

Comments suppressed due to low confidence (1)

src/id.rs:54

• The current implementation reassigns 'id' by trimming the string, resulting in a &str that borrows from a temporary String, which could lead to lifetime issues. Consider capturing the trimmed value as an owned String, e.g., 'let id = id.trim().to_owned();', to ensure it remains valid.

let id = id.trim();

[codecov]

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (d1dfee1) to head (5e025fc).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
src/id.rs	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
+ Coverage   89.64%   89.67%   +0.03%     
==========================================
  Files          37       37              
  Lines        3381     3401      +20     
  Branches     3381     3401      +20     
==========================================
+ Hits         3031     3050      +19     
  Misses        173      173              
- Partials      177      178       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dalonsoa]

I've a question/suggestion to improve readability, but this looks good, otherwise.

[dalonsoa]

Is there any reason for not using something like:

Suggested change

	for forbidden in FORBIDDEN_IDS.iter() {
	if id.eq_ignore_ascii_case(forbidden) {
	return Err(D::Error::custom(format!(
	"'{id}' is an invalid value for an ID"
	)));
	}
	}
	if ["all", "annual"].contains(&id) {
	return Err(D::Error::custom(format!(
	"'{id}' is an invalid value for an ID"
	)));
	}

There seems to be more succinct and readable ways of making this check: https://stackoverflow.com/a/72381091/3778792

[alexdewar]

It currently ignores case. You could convert id to lowercase then do the check, which would mean an extra allocation, though that's probably not the end of the world.

[alexdewar]

We could rewrite this with iterators, i.e. if FORBIDDEN_IDS.iter().any(..., but I'm not sure that would be much cleaner.

[dalonsoa]

OK!

[tsmbland]

Good idea

Buuut... maybe add some tests :)

[dalonsoa]

LGTM!

[dalonsoa]

OK!