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

Refactor builder #2062

Open
wants to merge 24 commits into
base: nightly
Choose a base branch
from
Open

Conversation

vanshady
Copy link
Contributor

Description

Builder module is too long, by creating a package and breaking it down, I've managed to reduce it to <2000 lines. More still could be done, but it's much more readable now with a still large constructor, but otherwise only core methods.

Issues Fixed or Closed

  • Fixes #(issue)

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code was submitted to the nightly branch of the repository.

@vanshady
Copy link
Contributor Author

spellcheck is failed because of CHANGELOG? Not related to my change

@YozoraXCII
Copy link
Contributor

spellcheck is failed because of CHANGELOG? Not related to my change

don't worry about that :)

@vanshady
Copy link
Contributor Author

Is there any config that enables everything that I can use to check if it works?

@chazlarson
Copy link
Contributor

There's a "kitchen sink" config in the json schema folder in nightly.

@vanshady
Copy link
Contributor Author

Tried with kitchen sink, other than collection issues as mentioned in #2065 it is working without errors now.

@YozoraXCII
Copy link
Contributor

Can I ask what the "purpose" of this PR is?

Does it provide any performance benefit, or is it purely streamlining the code?

@vanshady
Copy link
Contributor Author

Purely streamlining, unless you write the builder, no one can read a 4k lines monolithic class. It’s really against all kinds of best practice, so I’m just refactoring to make it easy to read and change in the future

@vanshady
Copy link
Contributor Author

Has anyone reviewed the PR and any feedback? This PR should have no actual logic change, just refactoring only. Also, I'm not python programmer by trade so hopefully I adhere to python best practice in general.

@chazlarson
Copy link
Contributor

It will be up to the author and repo owner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants