fix isTaxYearLeapYear

[elijah-wright]

in constants.xml, isTaxYearLeapYear is hardcoded for 2024 and 2028. this PR adds logic to see if a year is a leap year ([divisible by 4] or [divisible by 100 and 400]). it also adds a modulo operator to the fact graph to accomplish this

[elijah-wright]

because this is the first pull request and something many people will see, I would like to say that I learned from a former project manager at the IRS that development on Direct File has stopped since January. the source code is only public because of federal law. it's not likely that this is going to be merged but it's possible that the components of Direct File might be used elsewhere

edit: I have started work on OpenFile, a continuation of this project

[cjhskippy]

I love that you dove in and looked for a place to replace a TODO comment for a band-aid and tried to resolve the problem at its root.

Adding a modulo operator seems like an elegant way to do this. Love it. One other thing that would make this a really elegant fix would be to include testing to match what was made public.

Other comp nodes appear to have tests to run against them that survived the repo going public. (ex: direct-file/fact-graph-scala/shared/src/test/scala/gov/irs/factgraph/compnodes/DivideSpec.scala ) Should the modulo operator?

Closing out in a compliment sandwich, I'm impressed that you jumped in to a new space with limited documentation and took a first swing that's pretty strong.

[cjhskippy]

Should these be restricted just to IntNode ? Not very familiar with scala but I imagine it could get weird with non-integer types.

[elijah-wright]

other operators do limit the nodes that are usable, but there are three different types that could be used here: IntNode, DollarNode, and RationalNode. I really wasn't sure if this was going to be used later on so I just left it as an open type

[cjhskippy]

Makes sense. I may be overly cautious.

My thinking was that both the dollar and rational types would often contain decimal values, which in many cases are probably not intended inputs to a modulo. Being restrictive could be a nice warning signal if anyone tries to use this in a way that doesn't work by mistake. If it turns out someone needs to use decimal or dollar inputs later on (and has a better math understanding than me of how that would make sense), it'd be fine to push the work of adding that support to when there's a use case for it.

That being said since you have the only use of modulo anywhere, my feedback is entirely around hypothetical future use.

[fmhall]

LGTM

[jcsumlin]

LGTM

[elijah-wright]

😆 first time dealing with a force push, sorry if anyone got pinged

[ajharris96]

Just looking at this: ([divisible by 4] or [divisible by 100 and 400]). It is not correct. For example 2100 is not a leap year but is divisible by 4 so if your code matches that logic it would be wrong.

Should be (year % 4 == 0) and (year % 100 != 0 or year % 400 == 0)

[elijah-wright]

good catch lol, I changed it so that it looks more like ([divisible by 100 and divisible by 400] and [not divisible by 100 and divisible by 4])

[cjhskippy]

I don't think this currently builds. I still love the interest and ambition - adding a new scala operator is going straight into the deep end, and I think you're close.

For a PR that modifies the scala + fact dictionary, I think you'll want to check that the following steps work:

• in /direct-file/direct-file/df-client/df-client-app can you run npm run generate-fact-dictionary successfully?
• in /direct-file/direct-file/fact-graph-scala does sbt test pass?
• Have you run all the steps No. 1 through 15 in https://github.com/IRS-Public/direct-file/tree/main/direct-file/fact-graph-scala ?
• After having run the generate-fact-dictionary step, do the fact dictionary tests pass? (from /direct-file/direct-file/df-client/df-client-app/src/test run npm run test factDictionaryTests)
• Have you tested the four main branches of logic? (manual testing via the browser debugFactGraph.get() is probably best for this case. Because /taxYear is not "writable" I wasn't able to find a good way to automate a test for it.
  • /taxYear is not divisible by 4 (ex: 2025) => /isTaxYearLeapYear === false
  • /taxYear is divisible by 4 but not 100 (ex: 2024) /isTaxYearLeapYear === true
  • /taxYear is divisible by 100 but not 400 (ex: 2100) /isTaxYearLeapYear === false
  • /taxYear is divisible by 400 (ex: 2400) /isTaxYearLeapYear === true

I learned a lot from diving into this PR. I think I can help if you get stuck, or to pick this up if it feels like too much work.

[elijah-wright]

thank you, I didn't see where the testing was. I was able to get it to work and fix the tests

[cjhskippy]

Better! I still get errors when running npm run test factDictionaryTests that say 'When is not a registered CompNode'. I don't think When can follow Then. I think you'll only see When within Case within Switch. (I think that structure is defined in /direct-file/blob/main/direct-file/backend/src/main/resources/tax/FactDictionaryModule.rng)

One way to make the logic a little easier to follow and avoid the need for nested switch / case statements is to rely on short-circuit evaluation by listing the cases in this order:

• If the year is divisible by 400, it's a leap year.
• If it's divisible by 100, it's not a leap year (because you've already confirmed that it's not divisible by 400).
• If it's divisible by 4, it's a leap year (because you've already checked the special cases around centuries),
• In all other cases it's not a leap year

Happy to share as an example, but I think you've got this PR so close you deserve the glory for bringing it to a close.

[elijah-wright]

I rewrote it and I think that will work, none of the tests are running for me now so I don't know

[cjhskippy]

Lovely. Spotless complained about the formatting but after running spotless in the backend directory, all the tests passed and I was able to verify correct leap year behavior for 2024 (true), 2025 (false), 2100 (false), and 2400 (true). With those minor revisions, this will be a great first PR for anyone to look at who wants to modify the Direct File fact graph. This is a nice, practical use that covers some gnarly territory. ⭐

Branch with the spotless fixes applied: cjhskippy@f5509e1

[elijah-wright]

👍 ty, I cherry picked your commit to this branch

[cjhskippy]

Endorse. This makes me wonder what would have been possible if community engagement could have been part of Direct File's development.

As a former Direct File team member, I'm happy to say it looks like good work to me. 🎉

[elijah-wright]

the IRS added modulation in this commit so I'm closing this