codethesaur.us
codethesaur.us copied to clipboard
Refactor reference and compare views
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
Form
s more easily down the line
-
MetaInfo
handles loading multiple(language, version)
tuples & throws more descriptive errors instead ofKeyError
andFileNotFoundError
-
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
andlang2
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 getsconcept
&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!
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!
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.
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.
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.
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)
@cafce25 How's this coming along? Need any help with it?
How are you doing on this, @cafce25 ? Need anything from me?
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!
Hey @cafce25, you still there? I'm doing one last checkin on this. Let me know if you need anything!
@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 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.
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.