-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
Migrate internal code to use Direct Message terminology #1553
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ajnus Thanks for working on this! I haven’t reviewed the changes in detail.
But in the tests, ZT relies on gitlint
to enforce specific commit message rules. The README includes setup instructions for adding a gitlint commit-message hook, which lets you test locally and ensure your commit messages follow the required format.
While your commit messages generally look good, they don’t fully comply with gitlint’s formatting rules.
@@ -10,10 +10,10 @@ | |||
from zulipterminal.config.symbols import CHECK_MARK, MUTE_MARKER | |||
from zulipterminal.ui_tools.buttons import ( | |||
DecodedStream, | |||
DMButton, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, avoid using separate commits solely to fix test issues. Every test must pass on each commit since the Linting & tests / Ensure isolated PR commits (pull_request)
check ensures that all commits stand alone successfully.
Maybe try rebasing to correct issues in each commit as needed.
For example:
In the commit Refactor: replace 'PM' with 'DM' (ignoring model.py, test_model.py, f…)
, isort is not satisfied (which you fixed in this commit separately). Rebase, address this, and make sure each commit individually passes all tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My pleasure @rsashank 🫡
Indeed, the Linting & tests / Lint - Commit text format (gitlint) (pull_request)
was, at the time, the last test that needed a fixing - which I just did. :)
However, by fixing it - maybe that's just how it happens, still a newcomer into contributing don't really know yet - from 2 other tests that were skipped, now Linting & tests / Ensure isolated PR commits (pull_request)
do tests, and fails - the other last test still skipped, probably one test depends on the other and so on I imagine - so, the 1 commit per test passed was more a (yesterday) deadline (presentation) issue, although this structure does help me track which tests are good - even because from what I understand the tests only (re)run after a push to the repository is made - and which are not.
But overall in the end I'll just squash everything to what's better for you guys.
Stay tuned for the next episodes. =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! I just thought some clarification might be helpful since I assumed you were new to this. My mistake if I jumped in too early!
Good luck :)
0022d40
to
09ba862
Compare
@Ajnus Just a quick note re testing in general - these can all be run locally, as per the development notes in the README. In particular there is |
Hello @Ajnus, it seems like you have referenced #1344 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage. Please run An example of a correctly-formatted commit:
To learn how to write a great commit message, please refer to our guide. |
Doesn't this breaks |
Can't really. Even after |
1502677
to
de0e831
Compare
@Ajnus the commit history needs to be cleaned up before this will be ready for review. Check out our GitHub guide and commit guidelines for more details. |
@timabbott check the zulip chat please. |
1013cc0
to
7d5b898
Compare
refactor: Lint - Type consistency (mypy). refactor: Install & test - CPython 3.7 (ubuntu), codecov. refactor: Install & test - CPython 3.7 (ubuntu), codecov (2). refactor: Install & test - CPython 3.7 (ubuntu), codeco (3). refactor: Install & test - CPython 3.7 (ubuntu), codeco (4).
What does this PR do, and why?
Replaces instances of:
private, privates, _ private, private _, PRIVATE, PRIVATES, Private , pm, pms, _ pm, pm _, PM, Pm
(and derivatives, e.g: _is_private_message_to_self, private_msg_ids, private_msg_ids_by_user_ids, (...))
to Direct Message terminology, avoiding the
test_model.py
,model.py
files, strings and other instances related to the API, for API consistency reasons, while "exploring other parts", such as replacing comments.This PR consists of the - on-chat debated - first part of altering just internal code, not the second that focus on API related code.
External discussion & connections
#T1344 Migrate internal code to use Direct Message terminology
How did you test this?
Self-review checklist for each commit