Skip to content

feat: add roman command to convert numbers and Roman numerals (#91) - #126

Closed
KentSimon07 wants to merge 2 commits into
codepiyusss:mainfrom
KentSimon07:feat-roman-numerals
Closed

KentSimon07 wants to merge 2 commits into
codepiyusss:mainfrom
KentSimon07:feat-roman-numerals

Conversation

@KentSimon07

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds a fixit roman command that converts between numbers and Roman numerals. The direction is decided from the input: digits are converted to a numeral, anything else is parsed as a numeral.

  • New module fixit/roman.py with two pure functions (no printing):
    • to_roman(number): converts 1–3999 using a greedy lookup over the 13 symbol values (including subtractive pairs like CM, XC, IV)
    • from_roman(numeral): parses a numeral (case-insensitive), then validates it with a round-trip check (to_roman(result) must equal the input), which rejects malformed numerals such as IIII, VX and IC
  • Both raise ValueError for invalid input, so main() prints a friendly one-line error and exits with code 1
  • fixit/cli.py: added the roman subparser and run_roman, registered in COMMAND_TABLE like the existing commands

Example usage:

$ fixit roman 1994
MCMXCIV
$ fixit roman mcmxciv
1994
$ fixit roman 4000
Error: number must be between 1 and 3999
$ fixit roman IIII
Error: 'IIII' is not a valid Roman numeral

Related issue

Closes #91

Checklist

  • I commented on the issue first and was assigned
  • I removed the xfail marker (if fixing a bug) (N/A, this is a new feature)
  • I added or updated tests: 11 new tests in tests/test_roman.py (normal values, boundaries 1/3999/0/4000, lowercase, and invalid numerals)
  • pytest passes locally: 50 passed
  • This PR only addresses one issue

@KentSimon07

Copy link
Copy Markdown
Contributor Author

Hi @codepiyusss, I've finished this one: the roman command, validation and 11 tests, all passing. Ready for review whenever you have time. Could you assign me to #91? Thanks!

@codepiyusss

Copy link
Copy Markdown
Owner

Thanks for the contribution @KentSimon07 , this PR currently isn’t in a mergeable state.

There are two blockers:

  1. The branch has conflicts that need to be resolved before it can be merged.
  2. The CI and Ruff workflows are failing, so the change is not passing the repository checks.

Please fix the merge conflicts, address the failing CI/Ruff issues, and re-run the validation locally before submitting the PR again. Once the workflow checks pass and the conflict is resolved, we can review the feature again.

If you want, a good next step is to run:

  • git fetch origin
  • git rebase origin/main (or merge main into your branch)
  • pytest
  • ruff check .
  • ruff format --check .

After that, push the updated branch and open a new PR if needed.
Thanks!

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.

Feature: add a roman numeral command

3 participants