Skip to content

Conversation

@JonathanOppenheimer
Copy link
Member

Done by @noam987: Fixed by having the frontend call the server's backend

fixed by having the frontend call the server's backend
@JonathanOppenheimer
Copy link
Member Author

JonathanOppenheimer commented Oct 17, 2021

We need to resolve the merge conflict to move Noam's fixes into the main. I suggest using the dorm IP address for a separate environment key, maybe just "noam" or something with "development" still using localhost and then finally Jaims' server otherwise.
Screen Shot 2021-10-17 at 5 58 42 PM

(Easiest would probably be to pull changes from GitHub onto your branch, merge changes while accepting main's version and then changing the IP before repushing. Conflict should then be resolved).

Once this conflict is resolved and merged, we won't have to maintain two separate branches, and I'll make a PR from my fork with some other changes for review.

@JonathanOppenheimer
Copy link
Member Author

JonathanOppenheimer commented Oct 25, 2021

@noam987 I fixed your merge conflicts for you after merging the fixes @DitrusNight made to the front-end in #3 (which rendered some of my previous comments irrelevant). If you want to give this a test and see if your changes still work for multiple computer use, make whatever changes you need to make, approve this PR and then feel free to merge it.

@DitrusNight
Copy link
Contributor

DitrusNight commented Oct 26, 2021

It appears that there are two errors with the current status of the bug-fixing branch that you would like to address before merging.
Firstly, https://github.com/HelloWorldHackathon/My-CS-Plan/pull/2/files#diff-a8677778eedb20c80cc51976b37d9fc39e22c8b7092ce9e3494e0dbe00bc116eR26-R32 (app.py line 26-32)
A deep copy of app.data is being performed, however my previous pull request removed app.data and instead loaded the CSV for every request.
Either fix works. However I believe Noam's would work better since it doesn't perform file system reading operations for every request (like mine does, and like the original code did).
Therefore, we would probably want to re-implement app.data and load the CSV data in the initialization of the flask server. From there, we should do as Noam did, and create a deep copy for the csv data and use that for utils.compare_tracks

Secondly, https://github.com/HelloWorldHackathon/My-CS-Plan/pull/2/files#diff-999051457a1934c824a5f8395589ea5e749a581e9f70a43585c885319cb56e64R10545-R10559 (package-lock.json, line 10545-10559)
As we can see here, merge conflicts were not resolved, and so we will need to remove the extra lines made by the merge conflict.
Because we are merging both forms of the code, both parts should be kept. We just need to remove the extra merge conflict mess created by git.

@noam987
Copy link
Contributor

noam987 commented Oct 26, 2021

I'm going to do a full cleanup and look at this tomorrow.

@JonathanOppenheimer
Copy link
Member Author

The deep-copy change I was aware of and the line is still there, just admittedly useless - I added a comment for Noam to take a look as you're totally right on the deep_copy not being used at all. Noam's code is still viewable in the commit history for this PR so hopefully, it shouldn't be too difficult to create a "best mix."

Regarding the merge conflict, excellent catch, I completely missed that conflict and you're right that we'd want both! GitHub can be a total pain but hey this is why we don't commit directly to the main right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants