search

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

Orientation Project (Python for 24.SUM.A)
2 stars 6 forks source link

get experience by index & delete experience by index #31

Closed lh5844 closed 3 months ago

lh5844 commented 3 months ago

resolves issue #19 and issue #4

tests fail due to issue #15 not being resolved yet

andyjianzhou commented 3 months ago

Hey, this conflicts with #28. I fixed testcases and the get and post experiences implemented. Not the delete functionality though. I implemented it so that it saves and generates IDs, which helps you with #4

Pradyuman7 commented 3 months ago

Hey, this conflicts with #28. I fixed testcases and the get and post experiences implemented. Not the delete functionality though. I implemented it so that it saves and generates IDs, which helps you with #4

We can wait to merge that PR first then. What do you think @lh5844?

lh5844 commented 3 months ago

Alright that sounds good to me

Pradyuman7 commented 3 months ago

That PR has been merged, you can make the required changes now @lh5844

IvanKolchanov commented 3 months ago

Oh, I was looking at this code because I was implementing DELETE for /resume/skill and I feel like there is bug in the code. After deleting the experience on line 66 in app.py, you need to use "save_data('data/data.json', data)" to save the data, otherwise, there will be no change to the data.json file. And you should probably add checking the data to the test case too, so that it doesn't miss something like that. :-)

lh5844 commented 3 months ago

Thank you for the catch! I made the changes to delete request for experience

lh5844 commented 3 months ago

@Pradyuman7 this is ready for review

lh5844 commented 3 months ago

@Pradyuman7 Just made the changes, thank you for pointing them out