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

float.sh must print exactly three digits after the dot #241

Closed
wants to merge 6 commits into from

Conversation

LaithAlebrahim
Copy link
Contributor

No description provided.

@LaithAlebrahim
Copy link
Contributor Author

@yegor256 , Could you please review this issue here is the link of the ticket (#233)
I modified the code and added the required test also modifed the previous tests to handle the new feat.
Thanks

@yegor256
Copy link
Owner

@LaithAlebrahim try to run make clean test lint locally

@LaithAlebrahim
Copy link
Contributor Author

LaithAlebrahim commented Mar 22, 2024

Actually there is some failed test but not related in my code they are in different folder: because when I run make test TEST=tests/help/test-float.sh everything works fine
@yegor256
image
Here you can see the error in which metrics :
image

UPD: I've improved the checking here to be more accurate (ebe8071)

UPD2: Here is example of error which we need to modify:
(73f6a3f)
After modifications
image

@LaithAlebrahim
Copy link
Contributor Author

LaithAlebrahim commented Mar 22, 2024

@yegor256 I got the issue is that in the new float.sh we print .000 for all numbers as you required in task so we just need to modiffy other tests I can work on it and handle or u can make a ticket for it so I can submit it .
But the current float.sh and it's tests works fine as you see and required in task. Thanks

@yegor256
Copy link
Owner

@LaithAlebrahim you have to fix all tests in this branch, otherwise we won't be able to merge it

compose-dev.yaml Outdated
@@ -0,0 +1,12 @@
services:
Copy link
Owner

Choose a reason for hiding this comment

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

@LaithAlebrahim this file seems to not belong to this repo :)

@LaithAlebrahim
Copy link
Contributor Author

LaithAlebrahim commented Mar 22, 2024

@LaithAlebrahim you have to fix all tests in this branch, otherwise we won't be able to merge it

I'll delete sorry this because am running the project in docker dev environment , The project did not run rather than on this env.
But there is an issue that I tried to clone the original repo and got same failed test as in my branch
I followed this flow: Clone repo to dev Environmnet in docker-> make install -> pip install -r requirments.txt -> make clean test lint and it failed excatly at same point as in my code also when i tried to comment this tests it fails as well so I guess the problem not from my code .
image

This failed test from the original master branch in your repo. and same error on mine now so I guess we are on same page now. And the required from task is done cuz the float-test it passed
Thanks for understanding @yegor256
If you have any suggestions please let me know
If I should for example install any other lib or should download some datasets ..etc

@LaithAlebrahim
Copy link
Contributor Author

Could you please check my last comment and let me know if there is some thing need to be done .Thanks @yegor256

@yegor256
Copy link
Owner

@LaithAlebrahim you can run tests in Docker (if you can't install all dependencies locally):

docker build . -t cam
docker run --rm cam make test

However, I suggest you try again (a lot of changes have been made recently):

make install

@LaithAlebrahim LaithAlebrahim closed this by deleting the head repository Apr 5, 2024
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.

None yet

2 participants