codethesaur.us icon indicating copy to clipboard operation
codethesaur.us copied to clipboard

Refactor reference and compare views

Open cafce25 opened this issue 3 years ago • 7 comments

What GitHub issue does this PR apply to?

None

What changed and why?

  • Merge the views for language reference & comparision to reduce code duplication across them & their templates and make changing them easier. Added benefits are:

    • you can more easily add an additional language on the URL to compare instead of reference.
    • the new code supports comparing more than 2 languages (although this might not display pretty at too many languages).
    • using the same route to display and handle the form data allows us to switch to django Forms more easily down the line
  • MetaInfo handles loading multiple (language, version) tuples & throws more descriptive errors instead of KeyError and FileNotFoundError

  • Versions show in page titles & some more links point to the correct files on github (bottom of the page).

  • I made sure the old parameters lang1 and lang2 still work and did the same for the old routes /compare and /reference to ensure old links don't break, but since we don't need to differentiate between the both views index now renders them if it gets concept & lang parameters.

  • Version defaults to latest instead of showing an error if you don't provide one

  • unify all *friendly_name and *name variables into *name

  • unify all *key and *id variables into *key

Checklist

  • [x] I claimed any associated issue(s) and they are not someone else's
  • [x] I have looked at documentation to ensure I made any revisions correctly
  • [x] I tested my changes locally to ensure they work
  • [x] (If editing Django) I have added or edited any appropriate unit tests for my changes

Any additional comments or things to be aware of while reviewing?

Oof, those are a lot of random changes to the code, but they all sort of naturally evolved from merging the two views and I had a hard time separating them into their own PRs... If you disagree please let me know and I'll try to split this up into smaller chunks!

cafce25 avatar Feb 25 '22 16:02 cafce25

I'm commenting to let you know that I know this is here and I haven't not-noticed it, just haven't sat down to look at it yet as I wanted to finish my own super outdated PR.

Also one thing that needs to happen soon is a rewrite of the docs that are out there. A lot of changes recently have fundamentally changed how contributing to CT work, and I need to update that. I'd gladly take your help on that too since you've helped do a lot of refactoring and reconfiguring lately!

geekygirlsarah avatar Mar 07 '22 06:03 geekygirlsarah

I've looked this over and like the general idea of it. I'll look at the merge conflicts later and see what needs to get modified.

geekygirlsarah avatar Mar 10 '22 01:03 geekygirlsarah

A point before we merge this, should we instead of just rendering the reference/compare view for the legacy routes respond with 301 Moved permanently so that the browser follows it and maybe updates any bookmarks? That way we might drop/reuse them later down the road.

cafce25 avatar Mar 16 '22 15:03 cafce25

It's been a busy time (day? week? month?) I'm coming back around to this...

A point before we merge this, should we instead of just rendering the reference/compare view for the legacy routes respond with 301 Moved permanently so that the browser follows it and maybe updates any bookmarks? That way we might drop/reuse them later down the road.

My ultimate goal was URLs like /compare/csharp/python, though with the evolution now, perhaps a /compare/csharp/9/python/3 or something. So I guess I'd rather have them HTTP 302 Moved Temporarily until this is in place.

geekygirlsarah avatar Mar 28 '22 22:03 geekygirlsarah

I tried merging the conflict. You changed too much in here that I'm having a hard time doing it. The meta/io.json is easy, urls.py was pretty easy too, but views.py has too much going on. You should probably do that.

(Also may need you to make smaller PRs from now on to make some smaller changes at a time, partly for reviewing sake, partly in case something needs to roll back it's a bit easier to do that on a smaller PR)

geekygirlsarah avatar Mar 28 '22 22:03 geekygirlsarah

@cafce25 How's this coming along? Need any help with it?

geekygirlsarah avatar May 21 '22 00:05 geekygirlsarah

How are you doing on this, @cafce25 ? Need anything from me?

geekygirlsarah avatar Aug 05 '22 19:08 geekygirlsarah

Hey @cafce25, figured I'd check on this again. How's it going? How can I help? I know there's a few conflicts, let me know if I can help resolve anything!

geekygirlsarah avatar Sep 25 '22 22:09 geekygirlsarah

Hey @cafce25, you still there? I'm doing one last checkin on this. Let me know if you need anything!

geekygirlsarah avatar Oct 02 '22 12:10 geekygirlsarah

@cafce25 I haven't heard from you since March, so I'm going to close this. Also I think the merge conflicts will just be a pain.

You're entirely welcome to reopen and resume working if you want, but I just want to free this up from open stuff so I can manage things easier. Just let me know!

geekygirlsarah avatar Oct 04 '22 17:10 geekygirlsarah

@geekygirlsarah Yeah sorry for dropping the ball & not responding, had some stuff to work through. I'm back in the game though and have resolved all the merge conflicts, I can't seem to reopen the PR though. Maybe you can? Otherwise just let me know and I'll create a new one.

cafce25 avatar Oct 14 '22 13:10 cafce25

It looks like I can't reopen it for some reason (it's disabled for me) so not sure if it's something to do with the original branch or something. Sorry about that! You'll have to make a new one.

geekygirlsarah avatar Oct 15 '22 19:10 geekygirlsarah