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

Adding get drivers() #45

Merged
merged 23 commits into from
Oct 5, 2022
Merged

Adding get drivers() #45

merged 23 commits into from
Oct 5, 2022

Conversation

Exoutia
Copy link
Contributor

@Exoutia Exoutia commented Oct 4, 2022

Description

  • I have added the get_drivers function with option to add a round numbers as parameters.
  • This function will return the a panda df with name, permanent number, nationality and date of birth column.
  • As there is no permanent number before 2014 (according to api documentation) the function will return none in permanent number column if year is less than 2014 in parameter

Related Issue(s)

Fixes #41

Screenshots (If necessary)

for 2014

image

below 2014

image

@Exoutia Exoutia requested a review from alec-kr as a code owner October 4, 2022 08:28
@alec-kr
Copy link
Owner

alec-kr commented Oct 4, 2022

Great work 💯
Good catch on that permanent number issue too 😁

Some changes that must be made before merging:

  • The table in the README should also be updated to reflect the new function addition. You can add a new row containing the function call, and add a description and returned data type.

  • It doesn't seem as though you have added any test for the new function :(
    You should create a test similar to the ones found in /tests.

In /tests, you can create a file called test_get_drivers.py, which will test all methods in DriverInfo. Test data must also be added in /tests/test_data. For the test data, you can use a JSON file containing 3 rows returned from the get_drivers function.

If you have a look at the other tests in the folder, you should get an idea of how the file should be written.

Copy link
Owner

@alec-kr alec-kr left a comment

Choose a reason for hiding this comment

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

Pylint is not happy with some of the code format.

image

  • You must remove the trailing whitespaces from lines 6, 10, 14, 18 and 21 of driver_info.py
  • You must also add a class docstring to the DriverInfo class. Great work on adding the other docstrings btw ;)
  • There are also a few trailing whitespaces in f1pystats.py at lines 33, 41, 43 and 47.

Your PR should be able to merge after the lint issues, README and method tests have been fixed :)

@Exoutia
Copy link
Contributor Author

Exoutia commented Oct 4, 2022

Ok I will do as you pointed out this is my first serious code contribution 😁😁 in open source I will fix them asap

@alec-kr
Copy link
Owner

alec-kr commented Oct 4, 2022

Oh wow. Congrats on making your first PR 🎇

@alec-kr
Copy link
Owner

alec-kr commented Oct 4, 2022

Great work on fixing the linter issues.

Don't forget to update the README and add your test for the new module. (See my comment, in case you missed it)

@Exoutia
Copy link
Contributor Author

Exoutia commented Oct 4, 2022

Great work on fixing the linter issues.

Don't forget to update the README and add your test for the new module. (See my comment, in case you missed it)

I added the test module and updated the read me

tests/test_driver_info.py Outdated Show resolved Hide resolved
tests/test_driver_info.py Outdated Show resolved Hide resolved
@Exoutia
Copy link
Contributor Author

Exoutia commented Oct 5, 2022

Sorry for these small mistake I was doing these changes at 1 AM in my timezone so I think i was sleepy 🥲🥲 didn't notice them and in my jupyter note I was mostly relying on auto complete so I didn't find any error during my test run, again sorry I have made those changes as you have suggested.

@alec-kr
Copy link
Owner

alec-kr commented Oct 5, 2022

Sorry for these small mistake

It's okay. You can't always get everything right on the first try :)

Copy link
Owner

@alec-kr alec-kr left a comment

Choose a reason for hiding this comment

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

There are some changes that need to be made to pass the Pylint checks:

image

@Exoutia
Copy link
Contributor Author

Exoutia commented Oct 5, 2022

Maintainer works seems very tiring 😢
Still this is important work thanks for doing the hard work 👍

@alec-kr
Copy link
Owner

alec-kr commented Oct 5, 2022

Maintainer works seems very tiring

This is the first time I've been maintaining a project. It can be tiring at times.

thanks for doing the hard work

Thank you for contributing 😁

Copy link
Owner

@alec-kr alec-kr left a comment

Choose a reason for hiding this comment

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

Great work! I will merge this shortly

@alec-kr alec-kr merged commit ae25be6 into alec-kr:main Oct 5, 2022
@alec-kr alec-kr added the hacktoberfest-accepted Hacktoberfest PR has been accepted label Oct 5, 2022
@Exoutia Exoutia deleted the adding-get_drivers() branch October 5, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest PR has been accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add get_drivers() function
2 participants