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

Renamed PRIMARY_HOSTNAME to BOX_HOSTNAME #2465

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tognee
Copy link

@tognee tognee commented Dec 24, 2024

While I was working on the new domain management I found that the use of primary while talking about hostname or domains was getting a bit too confusing, so I decided to rename all things related to "primary" to something that makes more sense given the context around the code.

This is a big change in the codebase and I don't want it to be merged together with the new domains system, so I'm making a pull request just for this.

I've also added ruff files for linting and a dev-requirements.txt file with the python requirements found in setup/management.sh so that devs can create a virtualenv, install from the file and have type hints when using editors like vscode.

using "primary" to describe the domain of the box / mail server is confusing when working with multiple domains.
Usually the box domain is different from the domain you want to host your mail for.
@JoshData
Copy link
Member

I really can't foresee ever taking the time to review this.

@tognee
Copy link
Author

tognee commented Dec 24, 2024

Is there anyone else on the project that could take time to review it?

@yodax
Copy link
Contributor

yodax commented Dec 24, 2024

This is too large for too little gain. A cursory glance also shows python renames and not just the global var. This feels like a change that is high risk for just a rename. Also I’m familiar with the old name. So there would be some relearning when reviewing future PRs

@tognee
Copy link
Author

tognee commented Dec 24, 2024

I completely understand that this is a big change, but making the structure more manageable and making the variable names have a meaning is really important to help new developers familiarize with the project and understand how it works.

I don't think I'm able to work on the new feature without doing some refactoring, as the next PR would have had a split and refactor of the management/mailconfig.py file. That would've been a big change too, so maybe it's better for me to drop this.
I don't think I can make a good implementation of my idea without changing too much code.

@JoshData
Copy link
Member

I don't think I can make a good implementation of my idea without changing too much code.

This project is in a phase where high-risk changes are extremely difficult to get past the finish line. The folks who worked on the quota PR have been incredibly patient in waiting years to get through review.

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