TASK: Add Measurement class (#18)#46
TASK: Add Measurement class (#18)#46P-r-e-m-i-u-m wants to merge 6 commits intoTheGittyPerson:mainfrom
Measurement class (#18)#46Conversation
P-r-e-m-i-u-m
force-pushed
the
feature/measurement-class
branch
from
63c9ac2 to
5d6034d
Compare
TheGittyPerson
added
feature
TheGittyPerson
force-pushed
the
feature/measurement-class
branch
from
5d6034d to
b046bdf
Compare
|
I updated your branch with a rebase 👍 |
TheGittyPerson
force-pushed
the
feature/measurement-class
branch
from
b046bdf to
f9e264b
Compare
|
@P-r-e-m-i-u-m I just left a comment for @Poorna-Chandra-D in #48, but if he's unable to respond within a day or two, you may incorporate aspects of his |
|
@P-r-e-m-i-u-m confirmed from my side: you are welcome to reuse any relevant parts of my Measurement implementation and the review suggestions from #48 in this PR. I support #46 being the primary PR for this task. |
TheGittyPerson
force-pushed
the
feature/measurement-class
branch
from
31f8abd to
42a8b41
Compare
|
Updated your branch with a rebase 👍 will be reviewing your new changes soon |
TheGittyPerson
left a comment
TheGittyPerson
left a comment
There was a problem hiding this comment.
Here are some more suggestions for improvement. Please do ask questions if you have any
| if unit not in VALID_UNITS: | ||
| raise ValueError( | ||
| f"'{unit}' is not a recognised unit. " | ||
| f"Valid units are: {', '.join(sorted(VALID_UNITS))}." |
There was a problem hiding this comment.
VALID_UNITS only includes the shortened units (m, cm, ft, ...) and not their canonical forms (meters, centimeters, feet, ...). Consider adding those to the outputted list or mention something about it somewhere in the error message.
| def set_height(self, value: float, unit: str = "m") -> None: | ||
| """Set the person's height as a Measurement. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Consider moving this to the new Physical dataclass above
|
|
||
|
|
||
| @total_ordering | ||
| class Measurement: |
There was a problem hiding this comment.
What do you think about renaming the class to Length or Distance as the class only covers physical lengths?
Also, to prove you're actually reading these review comments and are not using AI or anything, leave a comment under your PR addressing my idea. Also include the name of your favorite cartoon, TV show or movie. Sorry if this is a bit wierd, but I want to be completely sure, since you previously mentioned your code is completely written by yourself.
| } | ||
| return self.value * conversions[self.unit] | ||
|
|
||
| def to_unit(self, unit: str) -> "Measurement": |
There was a problem hiding this comment.
Add a clean type check for unit
| """Check equality by comparing values in metres.""" | ||
| if not isinstance(other, Measurement): | ||
| return NotImplemented | ||
| return self.to_metres() == other.to_metres() |
There was a problem hiding this comment.
In Poorna's PR I did say to use exact comparison, but now that I think about it, floating-point conversions between units can yield tiny differences and make logically equal values compare as unequal. If we expect comparisons across units, consider using math.isclose with a tolerance.
| conversions = { | ||
| "m": 1.0, | ||
| "cm": 100.0, | ||
| "mm": 1000.0, | ||
| "ft": 3.28084, | ||
| "in": 39.3701, | ||
| } |
There was a problem hiding this comment.
You have this in both to_metres as well with slightly different constants. This is maintainability risk, so I recommend a single base map in metres and get the inverse when needed.
|
|
||
| def __str__(self) -> str: | ||
| """Return a string representation of the measurement.""" | ||
| return self.describe() |
There was a problem hiding this comment.
describe() rounds to 2 decimal places. I'd say make formatting configurable (much better) or keep full precision for __str__ while describe() can use rounding.
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com> # Conflicts: # src/person.py # Conflicts (second rebase conflict res): # theperson/measurement.py # theperson/person.py # Conflicts (third rebase conflict res): # theperson/person.py
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
TheGittyPerson
force-pushed
the
feature/measurement-class
branch
from
42a8b41 to
3d2acbf
Compare
|
@TheGittyPerson! I feel like keeping it as Measurement makes more sense since it’s a general concept and could support things like weight or temperature later on. I’m also addressing all the review points below. |
|
if you tell me i will fix the code ok |
…nit type check Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
|
wait |
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
|
It was tough bro 😅 but nice. That’s why you gave this to me, right? |
|
It was mostly because you asked for it but sure 🙌 |
|
This pull request has been inactive for some time, and will therefore be marked as 'stale'. |
3 participants
Closes #18
Added a Measurement class to store a value with a unit, and integrated it into Person.
Changes:
src/measurement.pywithMeasurementclassto_metres(),to_unit(),describe(), and__eq__()methodsPerson.heighttype fromfloattoMeasurement | Noneset_height()method toPersonintroduce()to useMeasurement.describe()