Skip to content

TASK: Add Measurement class (#18)#48

Open
Poorna-Chandra-D wants to merge 5 commits intoTheGittyPerson:mainfrom
Poorna-Chandra-D:feature/measurement-class
Open

TASK: Add Measurement class (#18)#48
Poorna-Chandra-D wants to merge 5 commits intoTheGittyPerson:mainfrom
Poorna-Chandra-D:feature/measurement-class

Conversation

@Poorna-Chandra-D
Copy link
Copy Markdown
Contributor

@Poorna-Chandra-D Poorna-Chandra-D commented Mar 16, 2026

Summary

Adds a Measurement class to represent a numeric measurement with a unit (meters, centimeters, feet, inches), and integrates it into the Person class to replace the bare float height attribute.

Changes

  • New file src/measurement.py — adds Measurement class with:
    • value and unit attributes
    • Validation for non-negative values and recognized units (meters, centimeters, feet, inches)
    • __str__ returning a human-readable representation (1.8 meters)
    • __repr__ for developer convenience
  • Updated src/person.py:
    • height attribute type changed from float | None to Measurement | None
    • introduce() displays self.height naturally via str(Measurement) output
    • New set_height(value, unit) method for convenient height assignment.

Usage

from person import Person
from measurement import Measurement

p = Person(name="Alice", height=Measurement(1.7, "meters"))
p.set_height(170, "centimeters")
p.introduce()
# I am 170 centimeters tall.

Closes #18

@TheGittyPerson TheGittyPerson added feature New feature or enhancement task This is a task issue / PR completing a task labels Mar 16, 2026
@Poorna-Chandra-D Poorna-Chandra-D force-pushed the feature/measurement-class branch from 0002fff to b6d0849 Compare March 16, 2026 03:02
@TheGittyPerson TheGittyPerson added the duplicate This issue or pull request already exists label Mar 16, 2026
@TheGittyPerson
Copy link
Copy Markdown
Owner

@Poorna-Chandra-D Since this is a duplicate of #46 (@P-r-e-m-i-u-m submitted the PR first but you started working first, I thought it would be unfair to choose one), are you interested in working with @P-r-e-m-i-u-m?

It's totally fine if you prefer to work solo (@P-r-e-m-i-u-m has already worked on many PRs so don't feel guilty). If you want to collaborate with him, you may discuss with him in his PR (or tag him here).

@TheGittyPerson TheGittyPerson force-pushed the feature/measurement-class branch from 171f367 to 1ef13e7 Compare March 17, 2026 16:32
@TheGittyPerson
Copy link
Copy Markdown
Owner

Updated your branch 👍

@TheGittyPerson TheGittyPerson force-pushed the feature/measurement-class branch from 1ef13e7 to da2cc7a Compare March 19, 2026 08:02
- Add src/measurement.py with Measurement class that stores a value and unit
- Measurement supports meters, centimeters, feet, and inches
- Updated Person.height to use Measurement type instead of float
- Added Person.set_height() method to set height with unit support
- Updated introduce() to display height using Measurement string representation

Closes TheGittyPerson#18

# Conflicts:
#	theperson/measurement.py
#	theperson/person.py

# Conflicts (2nd rebase conflict):
#	theperson/person.py
@TheGittyPerson TheGittyPerson force-pushed the feature/measurement-class branch from da2cc7a to beb8603 Compare March 22, 2026 12:06
Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
Copy link
Copy Markdown
Owner

@TheGittyPerson TheGittyPerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR. This is a very well-written module and there are minimal errors. Here are a few improvement suggestions I have for you:

Comment thread theperson/measurement.py Outdated
Comment thread theperson/measurement.py
Comment thread theperson/measurement.py Outdated
Comment thread theperson/measurement.py
Comment thread theperson/person.py
@TheGittyPerson TheGittyPerson changed the title TASK: Add Measurement class (#18) TASK: Add Measurement class (#18) Mar 23, 2026
@TheGittyPerson
Copy link
Copy Markdown
Owner

@Poorna-Chandra-D are you able to address my review comments? If you're busy or unable to do so, I completely understand 👍

@Poorna-Chandra-D
Copy link
Copy Markdown
Contributor Author

Thanks for checking in. I am okay with @P-r-e-m-i-u-m incorporating my Measurement updates and your review suggestions into #46. Since #48 is a duplicate, feel free to proceed with #46 as the main PR.

@Poorna-Chandra-D
Copy link
Copy Markdown
Contributor Author

@TheGittyPerson confirming: the commented review points have been addressed in the updated Measurement implementation (VALID_UNITS now includes aliases, comparison behavior is consistent with exact equality/ordering, and total_ordering is used so <=/>=/ > are supported).

@P-r-e-m-i-u-m
Copy link
Copy Markdown
Contributor

"Incorporated review feedback from #48 — rejected bool in value check, added lt and @total_ordering for full comparison support. Ready for re-review @TheGittyPerson 👍"

Copy link
Copy Markdown
Owner

@TheGittyPerson TheGittyPerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some more suggestions for improvement. Please do ask if you have any questions

@TheGittyPerson
Copy link
Copy Markdown
Owner

TheGittyPerson commented Mar 27, 2026

Please ignore the above change request comment, it was meant to go to Premium's PR (I got confused because the titles are the same lol)

@github-actions
Copy link
Copy Markdown

This pull request has been inactive for some time, and will therefore be marked as 'stale'.
Tag a maintainer if you wish to discuss this further.

@github-actions github-actions bot added the stale No activity here in a while label Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists feature New feature or enhancement stale No activity here in a while task This is a task issue / PR completing a task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TASK: Add Measurement class

3 participants