-
-
Notifications
You must be signed in to change notification settings - Fork 278
Support development using makefile #777
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
Changes from all commits
cfdda12
a2f48ee
c150641
0a722c6
68b6c4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
.PHONY: install-devel check lint test check-clean-tree fix force-fix venv | ||
|
||
# NOTE: ZT_VENV and BASEPYTHON are advanced undocumented features | ||
# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>" | ||
|
||
BASEPYTHON?=python3 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the only restriction this poses is being able to choose the exact python version during venv creation. This isn't a limitation on its own, but if we are going for an "advanced" setup option, we should consider this being customizable. |
||
ZT_VENV?=zt_venv | ||
neiljp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
VENV_ACTIVATE=. $(ZT_VENV)/bin/activate | ||
PYTHON=${ZT_VENV}/bin/$(BASEPYTHON) | ||
|
||
SOURCES = zulipterminal tests setup.py | ||
|
||
# Default target at top | ||
install-devel: venv | ||
|
||
### LINT/TEST FILES ### | ||
|
||
check: lint test | ||
|
||
lint: venv | ||
@tools/lint-all | ||
|
||
test: venv | ||
@pytest | ||
|
||
### FIX FILES ### | ||
|
||
check-clean-tree: | ||
@git diff --exit-code --quiet || (echo 'Tree is not clean - commit changes in git first' && false) | ||
|
||
fix: check-clean-tree | ||
@make force-fix | ||
|
||
force-fix: venv | ||
@echo "=== Auto-fixing files ===" | ||
isort --recursive $(SOURCES) | ||
autopep8 --recursive --in-place $(SOURCES) | ||
autoflake --recursive --in-place $(SOURCES) | ||
|
||
### VENV SETUP ### | ||
# Short name for file dependency | ||
venv: $(ZT_VENV)/bin/activate | ||
|
||
# If setup.py is updated or activate script doesn't exist, update virtual env | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should perhaps include the fact that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean if you don't use a custom environment name for all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it seems to build the standard one without that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, since the custom one is a bit awkward to run and manage, perhaps we should only promote the standard one. |
||
$(ZT_VENV)/bin/activate: setup.py | ||
@echo "=== Installing development environment ===" | ||
test -d $(ZT_VENV) || $(BASEPYTHON) -m venv $(ZT_VENV) | ||
$(PYTHON) -m pip install -U pip && $(PYTHON) -m pip install -e .[dev] && touch $(ZT_VENV)/bin/activate |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,15 +283,15 @@ def test_set_narrow_not_already_set(self, model, initial_narrow, narrow, | |
} | ||
}, {0, 1}), | ||
([['stream', 'FOO'], | ||
['topic', 'BOO']], { | ||
['topic', 'BOO']], { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that Otherwise, it could potentially accumulate a bunch of unrelated changes in PRs which build on a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the difference is that autopep8 fixes many pep8 rules as specified, but also some additional ones as here: https://pypi.org/project/autopep8/#features We could add this to CI without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. :/ Nonetheless, we should perhaps enforce this with our CI for consistency. |
||
'topic_msg_ids': { | ||
1: { | ||
'BOO': {0, 1} | ||
} | ||
} | ||
}, {0, 1}), | ||
([['stream', 'FOO'], # Covers one empty-set case | ||
['topic', 'BOOBOO']], { | ||
['topic', 'BOOBOO']], { | ||
'topic_msg_ids': { | ||
1: { | ||
'BOO': {0, 1} | ||
|
@@ -859,11 +859,11 @@ def test__handle_message_event(self, mocker, user_profile, response, | |
@pytest.mark.parametrize(['topic_name', 'topic_order_initial', | ||
'topic_order_final'], [ | ||
('TOPIC3', ['TOPIC2', 'TOPIC3', 'TOPIC1'], | ||
['TOPIC3', 'TOPIC2', 'TOPIC1']), | ||
['TOPIC3', 'TOPIC2', 'TOPIC1']), | ||
('TOPIC1', ['TOPIC1', 'TOPIC2', 'TOPIC3'], | ||
['TOPIC1', 'TOPIC2', 'TOPIC3']), | ||
['TOPIC1', 'TOPIC2', 'TOPIC3']), | ||
('TOPIC4', ['TOPIC1', 'TOPIC2', 'TOPIC3'], | ||
['TOPIC4', 'TOPIC1', 'TOPIC2', 'TOPIC3']), | ||
['TOPIC4', 'TOPIC1', 'TOPIC2', 'TOPIC3']), | ||
('TOPIC1', [], ['TOPIC1']) | ||
], ids=['reorder_topic3', 'topic1_discussion_continues', 'new_topic4', | ||
'first_topic_1']) | ||
|
@@ -1321,7 +1321,7 @@ def test_update_star_status(self, mocker, model, event_op, | |
assert model.index['messages'][changed_id]['flags'] == flags_after | ||
(model._update_rendered_view. | ||
assert_has_calls([mocker.call(changed_id) | ||
for changed_id in changed_ids])) | ||
for changed_id in changed_ids])) | ||
|
||
for unchanged_id in (set(indexed_ids) - set(event_message_ids)): | ||
assert (model.index['messages'][unchanged_id]['flags'] | ||
|
@@ -1533,7 +1533,7 @@ def test__handle_subscription_event_mute_streams(self, model, mocker, | |
model.controller.update_screen.assert_called_once_with() | ||
|
||
@pytest.mark.parametrize(['event', 'expected_pinned_streams', | ||
'expected_unpinned_streams'], [ | ||
'expected_unpinned_streams'], [ | ||
( | ||
{ | ||
'property': 'pin_to_top', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,4 +272,3 @@ def handle_link(self, *_: Any) -> None: | |
""" | ||
Classifies and handles link. | ||
""" | ||
pass |
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.
We should mention that the
ZT_VENV=...
would not only be required for the first time but every time it is run.Additionally, perhaps rename
<command>
to<zt-command>
?Also, should we rather include this in the README so that it is easier to find?
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.
I had specifically left these notes in the makefile for now as they seemed more 'advanced', and may be scope for further extension - they work, but for now it's easier to just have one environment with a standard name.
I'm not sure we gain from
zt-command
- what matters is running the different makefile 'targets', which are more just specific to those in the makefile.