Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated the organic name when creating the object #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SasoriOlkof
Copy link

updated the organic name when creating the object

Now when you create a measurement object the unit is converted to the most optimal
for example, if you declare Capacitance('0.0001 F'), it returns the object Capacitance('100 uF')

I recommend calling the function self.update_org_name() after calculations or before printing the value on the screen

updated the organic name when creating the object

Now when you create a measurement object the unit is converted to the most optimal
for example, if you declare Capacitance('0.0001 F'), it returns the object Capacitance('100 uF')

I recommend calling the function self.update_org_name() after calculations or before printing the value on the screen
@codingjoe codingjoe marked this pull request as draft April 22, 2020 08:24
@codingjoe
Copy link
Collaborator

Hi @SasoriOlkof, that is a great idea! I'd love to see this included in version 4. Would you mind adding tests for the new function and maybe an integration test to verify the overall behavior. Once you are done and all checks pass, simply click the "Ready for review" button.
Best, Joe

@codingjoe codingjoe self-requested a review April 22, 2020 08:26
@SasoriOlkof SasoriOlkof marked this pull request as ready for review April 22, 2020 08:37
@SasoriOlkof
Copy link
Author

Hello,

Sorry, I don't know how the tests work 😁, what do I have to add?

@SasoriOlkof
Copy link
Author

something like that is enough?

@SasoriOlkof
Copy link
Author

I think that's fine now :)

Copy link
Collaborator

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

Hi @SasoriOlkof you are off to a good start however, there is still much to do.
Only Unit and MetricUnit implement Unit.factor, DegreeUnit for temperatures does not.
You also cast the factor to a float at one point. I don't think that wise, considering we use decimals for a reason here.

Also, plenty of other tests are failing. If you need any help, please let me know.

Best,
Joe

@SasoriOlkof
Copy link
Author

Hello,

What would you rather check if we have factor or move the functionality?

During the development and testing of this functionality I saw that using Decimal does not work, maybe instead of a dictionary, looping and rounding in each iteration, but it seems less efficient

image

image

@codingjoe
Copy link
Collaborator

I see, keep the floats then for now. Let me know if you need any assistance in fixing the remaining issues in the test suite. Please also remember to add tests for negative example, where you code should not be executed. Best Joe

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.

2 participants