Skip to content

routes.py and test_routes.py are HUGE #246

Closed

Description

@aaron-junot

In the past, it didn't bother me that routes.py and test_routes.py were both over 250 lines of code. However, we've gotten to the point where routes.py is close to 500 LOC and test_routes.py is almost 1000 (and after #244 is merged, it will be over 1000). It's time to go ahead and break these up into smaller files so we can more easily find what we're looking for.

I'm open to how it should be organized, no strong opinions yet. If I were to do it myself, I'd probably group it similar to the documentation where each section was in it's own file. So you'd have search_routes.py, resources_routes.py, categories_routes.py and languages_routes.py. Alternatively, there could be new directories for each of these and a routes.py in each of them, though I don't know if that makes a lot of sense because I imagine that routes.py is all that would go in those directories. The test files would be broken up similarly, though depending on how big each file is, they may need to be broken down further, but we won't know until we go in there and break them up and see what happens. I would hope that each would be less than 250 LOC but I honestly don't know.

Activity

  1. Jitsusama commented on Oct 26, 2019

    @Jitsusama
    Contributor

    @aaron-suarez; I'm willing to start tackling this. Would you mind if I do this on a separate PR per organizational boundary? I'm thinking of creating a routes package in place of the routes module, and then having a single module per functional area. The first PR would be groundwork preparing for this, and then the subsequent PRs would be for each functional area and all would be based on the preparatory PR. This should make it easier to review each PR, and, honestly, give me some Hacktoberfest cred 😉

  2. Jitsusama commented on Oct 26, 2019

    @Jitsusama
    Contributor

    Actually, I think I will have a base branch with the setup, and just do a separate PR for each boundary, instead of having a separate setup PR which probably wouldn't be that big anyways.

  3. Jitsusama commented on Oct 26, 2019

    @Jitsusama
    Contributor

    Annndddd, I'm realizing that it's going to be a mess splitting them up into separate PRs. Instead, I think I'll have one PR for routes cleanup and another for route test cleanup. Otherwise, it's probably going to be more confusing to actually go through the PRs, as they will all be dependent on each other (due to modifying the same file over and over again.)

  4. aaron-junot commented on Oct 26, 2019

    @aaron-junot
    MemberAuthor

    Yeah, I'd much prefer it to be one PR for routes and another PR for the tests. That way, we can verify that moving the routes doesn't break the tests (so do that one first). Then do the tests as a follow up so we can break it down but we can still have confidence that everything is still working fine.

  5. Jitsusama commented on Oct 26, 2019

    @Jitsusama
    Contributor

    Yeah, that's my current plan. Just waiting for feedback before tackling the merge conflicts.

  6. added a commit that references this issue on Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions