Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify modeling for builds and Sphinx context data #3103

Closed
agjohnson opened this issue Sep 14, 2017 · 6 comments
Closed

Simplify modeling for builds and Sphinx context data #3103

agjohnson opened this issue Sep 14, 2017 · 6 comments
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Milestone

Comments

@agjohnson
Copy link
Contributor

Considering our patterns for how we use model data in our builders, there are a few deficiencies:

  • The model code our builders use make use of the DONT_HIT_DB setting. But our builders should never hit the DB, they should always be API driven.
  • We hack up database modeling to mock out API Project objects, so that we can use some of the model methods. I feel like this should also be API driven, with all of these method instead becoming attributes, and the API returns these attributes hard coded
  • The context data is hodge podge mix of attributes. For instance, we don't pass on most of the project information, and we only pass on subproject slug and url, but no other fields. This should all be standardized, and perhaps, should just be data directly from our API
  • We pass a completely different set of data from the footer. This should be the same as the context data going in, and from the same source.

This will offer a few benefits:

  • If we standardize data, we can just drop the data as a dict directly into the Sphinx context. This allows us to also put the context data creation on a signal, to make this extensible.
  • Reduces confusion around builder being sometimes API driven, sometimes database driven. This is helpful for testing and replicating how we actually use the modeling
  • Allows for our stored context data, the data in the sphinx context, to match with the structure of the context we pass from the footer
  • The plan would be footer would be data driven, not just a blob of html. This is a separate thing. But data here would match the data already in the template

Solution

  • Convert methods without arguments to attributes on Project and return these selectively from the API.
  • Decide what to do with methods with arguments, if any. I lean towards making this API driven wherever we can. If this has to be a call on the API, i'm fine with that.
  • Consider what we need to return from the API for the build process (that is, full return) and what we return from the API for normal requests (for user data, and data into the Sphinx context). We can perhaps eliminate overlap
  • Make this a centralized class that both the footer data and Sphinx context rely on. Stop duplication of this code

This will take a larger audit of how we are using data from the API, and perhaps blends in with a larger API v3 project. I think we can keep this separate however.

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Sep 14, 2017
@agjohnson
Copy link
Contributor Author

Ping @ericholscher

@ericholscher ericholscher added the Feature New feature label Sep 14, 2017
@ericholscher
Copy link
Member

Agreed 100%. We use a lot of the project modeling though in the build, which is why it's hard to break this out historically. This is a good starting point to completely breaking the builder code out, which seems like a useful end game.

@ericholscher
Copy link
Member

I really want the builds to use other modeling instead of the Project, so it would be much more explicit what you can use that doesn't depend on hitting the DB.

@ericholscher
Copy link
Member

This also relates to #3054, and I believe the full list of build data required is here: https://github.com/rtfd/readthedocs.org/pull/3054/files#diff-fab3f6c80eb0a5d20dc58fae29ac388dR28

@agjohnson
Copy link
Contributor Author

Yeah, i was thinking the full build return Project API endpoint would just have the attribute fields appended there. I haven't done an audit to see how many argument methods we are using yet however.

I was also considering secondary modeling, though if we are able to make this 100% api driven, that might not be necessary. We'll just treat the objects as glorified dicts from Slumber. I can definitely see a case where we might want modeling though. I also started considering what a new API might look like, without getting too serious about it. I think the Way of The Future is to use OpenAPI and schema to auto generate a client. This would supplement the need for modeling on the client side -- again, if we're 100% api driven.

@agjohnson agjohnson added this to the Refactoring milestone Sep 19, 2018
@agjohnson agjohnson added Improvement Minor improvement to code and removed Feature New feature labels Sep 19, 2018
@humitos
Copy link
Member

humitos commented Feb 21, 2024

I don't think this issue is valid anymore. We are moving away from injecting data on the Sphinx context (see readthedocs/addons#72)

Also, I don't think we will change our internal Project and API modeling at this point. I would say that the next step here will eventually be "Isolating the build process" (as discussed in https://github.com/readthedocs/meta/discussions/135)

@humitos humitos closed this as completed Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

3 participants