Updated 3 methods to resolve issues

kritgrover commented 1 year ago

Adds new experience, skill or education and return index
Returns specific experience, skill or education
Returns list of experience, skill or education

wrussell1999 commented 1 year ago

Overall, good PR! @kritgrover! I'd recommend renaming ski to skill for improved readability (as exp and edu are shorthand for those longer terms)!

Do you think you can write any additional tests to test the functionality? Maybe a test for checking your index logic works too?

wrussell1999 commented 1 year ago

In future @kritgrover, make sure to reference the issues you've implemented:

Some fellows had already started contributing to them as they thought no one else had assigned themselves. In future, make sure to assign yourself to avoid duplicate work!

kritgrover commented 1 year ago

sure, i'll make the changes and keep that in mind!

Henoven commented 1 year ago

Hey well done! @kritgrover I see you're on fire with issues. I agree with @wrussell1999 and I wanted to give you another suggestion. Normally submit PR per issue but in this case it makes sense to do all together : )

wrussell1999 commented 1 year ago

@kritgrover looking good with the tests. Can you write one last test:

Add a new experience and get the index.
Make a request with that index and check it returns the same JSON

wrussell1999 commented 1 year ago

Hey @kritgrover. Looks like an error on our side meant Pylint didn't run. Now it's merged into main, it has failed. Can you run pylint on your code and update the styling as required. Make a new PR to submit this

kritgrover commented 1 year ago

Hey @wrussell1999, sorry for that! Just submitted the new PR with the required changes.

MLH-Fellowship / orientation-project-python-23.SUM.A

Updated 3 methods to resolve issues #24