Skip to content

Conversation

@eddierubeiz
Copy link
Contributor

One of several small refactors that were unrelated to my main quest in ref #2787

@eddierubeiz eddierubeiz changed the title Simplify fixity report template by assigning @stale_in_days once in the controller [WIP] Simplify fixity report template by assigning @stale_in_days once in the controller Oct 16, 2025
@eddierubeiz eddierubeiz marked this pull request as ready for review October 17, 2025 18:58
@eddierubeiz eddierubeiz changed the title [WIP] Simplify fixity report template by assigning @stale_in_days once in the controller Simplify fixity report template by assigning @stale_in_days once in the controller Oct 17, 2025
@jrochkind
Copy link
Contributor

It's not clear to me why this simplifies anything. If we wanted to change the value, we had one place to change it before, and one place to change it now.

There might be something I'm missing the change or not considering! Can you say what do you see as the simpfliication of putting the constnat in an iVar instead of referring to the constant directly?

@jrochkind
Copy link
Contributor

Also if the constant FixityReport::STALE_IN_DAYS still exists (I don't see it being removed here?), do we now have two constants which have to match, FixityReport::STALE_IN_DAYS, and ScihistDigicoll::AssetsNeedingFixityChecks::DEFAULT_PERIOD_IN_DAYS? Or did we already have that and now still do?

@eddierubeiz
Copy link
Contributor Author

eddierubeiz commented Nov 5, 2025

No, the PR does in fact remove STALE_IN_DAYS; The main motivation of this PR, in fact, is to get rid of it, and it's the only change we're making to FixityReport in this PR.

STALE_IN_DAYS is never used at all in FixityReport. It doesn't make sense that there would be a constant defined there based on another constant in another class, and then never used.

The value IS used, however, in the report template, six times, in the descriptions of the statistics. So the template has to get the value from somewhere. I definitely know I don't want use FixityReport::STALE_IN_DAYS.

I could of course repeatedly evaluate ScihistDigicoll::AssetsNeedingFixityChecks::DEFAULT_PERIOD_IN_DAYS in the template, but that's clunky, so instead I assigned an ivar once in the controller and reused it six times in the template.

@jrochkind
Copy link
Contributor

I'm not seeing the benefit of replacing a local constant (set to the value of another constant) with a local ivar (set to the value of another constant). It is indeed constant, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants