-
Notifications
You must be signed in to change notification settings - Fork 2
feat: overall progress, completion celebration, first-launch welcome (#14, #15, #16) #31
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
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,45 @@ | ||||||||||
| # pythonlings/screens/welcome.py | ||||||||||
| from __future__ import annotations | ||||||||||
|
|
||||||||||
| from textual.app import ComposeResult | ||||||||||
| from textual.binding import Binding | ||||||||||
| from textual.containers import Vertical | ||||||||||
| from textual.screen import ModalScreen | ||||||||||
| from textual.widgets import Static | ||||||||||
|
|
||||||||||
| from pythonlings.core.state import save as save_state | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def welcome_text() -> str: | ||||||||||
| """The first-launch explainer for the edit/save/advance loop.""" | ||||||||||
| return ( | ||||||||||
| "You learn Python here by fixing small broken programs. The loop is:\n\n" | ||||||||||
| " 1. Edit the current exercise in the built-in editor.\n" | ||||||||||
| " 2. Save -- the check reruns automatically as you type.\n" | ||||||||||
| " 3. Remove the `# I AM NOT DONE` marker to advance to the next one.\n\n" | ||||||||||
|
Comment on lines
+18
to
+19
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. Clarify the onboarding loop sentence to remove contradictory behavior. Line 18 says users should save, then says checks rerun “as you type,” which conflicts with the save-driven loop described in the PR objectives. Proposed copy tweak- " 2. Save -- the check reruns automatically as you type.\n"
+ " 2. Save -- checks rerun automatically.\n"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| "Handy keys: F1 hint - F3 exercise list - F4 topics - " | ||||||||||
| "F5 local docs - Ctrl+Q quit.\n\n" | ||||||||||
| "Press Enter to start." | ||||||||||
| ) | ||||||||||
|
|
||||||||||
|
|
||||||||||
| class WelcomeScreen(ModalScreen[None]): | ||||||||||
| """First-launch onboarding overlay explaining the core loop.""" | ||||||||||
|
|
||||||||||
| BINDINGS = [ | ||||||||||
| Binding("enter", "start", "Start", priority=True), | ||||||||||
| Binding("escape", "start", "Start"), | ||||||||||
| ] | ||||||||||
|
Comment on lines
+29
to
+32
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. 🧩 Analysis chain🏁 Script executed: cat -n pythonlings/screens/welcome.py | sed -n '25,35p'Repository: abhiksark/pythonlings Length of output: 444 🌐 Web query:
💡 Result: The Ruff rule RUF012, titled "mutable-class-default," identifies mutable default values assigned to class attributes [1]. Such attributes are shared across all instances of the class, which can lead to unintended side effects where changes in one instance unexpectedly impact others [1]. While the rule is commonly triggered by simple assignments like Citations:
🏁 Script executed: rg "BINDINGS\s*=" pythonlings/ -A 2Repository: abhiksark/pythonlings Length of output: 912 🌐 Web query:
💡 Result: In the Ruff linter, RUF012 is a rule that identifies mutable default values in class attributes [1][2]. What it does: It flags class attributes that are assigned mutable data structures (such as lists, sets, or dictionaries) directly in the class body [1][2]. Because these objects are created when the class is defined, they are shared across all instances of that class, which often leads to unintended side effects when one instance modifies the shared state [1][2]. Why it is flagged: Mutable default values in class bodies are generally considered an anti-pattern because they create implicit shared state between instances [1]. Common fixes: - Initialize the attribute in the class's Citations:
Use a tuple instead of a list for the The list-based Suggested change- BINDINGS = [
+ BINDINGS = (
Binding("enter", "start", "Start", priority=True),
Binding("escape", "start", "Start"),
- ]
+ )🧰 Tools🪛 Ruff (0.15.17)[warning] 29-32: Mutable default value for class attribute (RUF012) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||||||
|
|
||||||||||
| def compose(self) -> ComposeResult: | ||||||||||
| yield Vertical( | ||||||||||
| Static("[bold]Welcome to pythonlings[/bold]", id="welcome-title"), | ||||||||||
| Static(welcome_text(), id="welcome-body"), | ||||||||||
| Static("Enter Start", id="welcome-footer"), | ||||||||||
| id="welcome-window", | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| def action_start(self) -> None: | ||||||||||
| self.app.state.seen_intro = True | ||||||||||
| save_state(self.app.root, self.app.state) | ||||||||||
| self.dismiss() | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,62 @@ | ||||||||||||||
| import shutil | ||||||||||||||
| from pathlib import Path | ||||||||||||||
|
|
||||||||||||||
|
Comment on lines
+1
to
+3
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. Add This new Python module is missing the required future-annotations import. Proposed fix+from __future__ import annotations
+
import shutil
from pathlib import PathAs per coding guidelines, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||
| import pytest | ||||||||||||||
| from textual.worker import WorkerCancelled | ||||||||||||||
|
|
||||||||||||||
| from pythonlings.app import PythonlingsApp | ||||||||||||||
| from pythonlings.core.state import State, load as load_state, save as save_state | ||||||||||||||
| from pythonlings.screens.track import TrackScreen | ||||||||||||||
| from pythonlings.screens.welcome import WelcomeScreen | ||||||||||||||
|
|
||||||||||||||
| MULTI = Path(__file__).parent.parent / "fixtures" / "multi_topic" | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _work_copy(tmp_path: Path) -> Path: | ||||||||||||||
| work = tmp_path / "work" | ||||||||||||||
| shutil.copytree(MULTI, work, ignore=shutil.ignore_patterns(".pythonlings")) | ||||||||||||||
| return work | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| async def _settle(pilot) -> None: | ||||||||||||||
| for _ in range(6): | ||||||||||||||
| try: | ||||||||||||||
| await pilot.app.workers.wait_for_complete() | ||||||||||||||
| except WorkerCancelled: | ||||||||||||||
| pass | ||||||||||||||
| await pilot.pause() | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @pytest.mark.asyncio | ||||||||||||||
| async def test_welcome_shown_on_first_launch(tmp_path: Path) -> None: | ||||||||||||||
| app = PythonlingsApp(root=_work_copy(tmp_path)) | ||||||||||||||
| async with app.run_test() as pilot: | ||||||||||||||
| await _settle(pilot) | ||||||||||||||
| assert isinstance(app.screen, WelcomeScreen) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @pytest.mark.asyncio | ||||||||||||||
| async def test_welcome_not_shown_after_intro_seen(tmp_path: Path) -> None: | ||||||||||||||
| work = _work_copy(tmp_path) | ||||||||||||||
| save_state(work, State(seen_intro=True)) | ||||||||||||||
|
|
||||||||||||||
| app = PythonlingsApp(root=work) | ||||||||||||||
| async with app.run_test() as pilot: | ||||||||||||||
| await _settle(pilot) | ||||||||||||||
| assert not isinstance(app.screen, WelcomeScreen) | ||||||||||||||
| assert isinstance(app.screen, TrackScreen) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @pytest.mark.asyncio | ||||||||||||||
| async def test_dismissing_welcome_persists_seen_intro(tmp_path: Path) -> None: | ||||||||||||||
| work = _work_copy(tmp_path) | ||||||||||||||
|
|
||||||||||||||
| app = PythonlingsApp(root=work) | ||||||||||||||
| async with app.run_test() as pilot: | ||||||||||||||
| await _settle(pilot) | ||||||||||||||
| assert isinstance(app.screen, WelcomeScreen) | ||||||||||||||
| await pilot.press("enter") | ||||||||||||||
| await _settle(pilot) | ||||||||||||||
| assert not isinstance(app.screen, WelcomeScreen) | ||||||||||||||
|
|
||||||||||||||
| assert load_state(work).seen_intro is True | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from pythonlings.screens.track import celebration_message | ||
|
|
||
|
|
||
| def test_celebration_message_includes_count_and_is_celebratory() -> None: | ||
| msg = celebration_message(292) | ||
|
|
||
| assert "292" in msg | ||
| assert "🎉" in msg |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from pythonlings.core.state import completed_count | ||
| from pythonlings.widgets.progress import format_progress | ||
|
|
||
|
|
||
| def test_format_progress_shows_topic_and_overall() -> None: | ||
| line = format_progress(2, 5, 10, 100) | ||
|
|
||
| assert "2/5" in line | ||
| assert "40%" in line # topic 2/5 | ||
| assert "10/100" in line | ||
| assert "10%" in line # overall 10/100 | ||
| assert "Overall" in line | ||
|
|
||
|
|
||
| def test_format_progress_handles_zero_totals() -> None: | ||
| line = format_progress(0, 0, 0, 0) | ||
|
|
||
| assert "0/0" in line | ||
| assert "0%" in line | ||
|
|
||
|
|
||
| def test_completed_count_ignores_names_not_in_curriculum() -> None: | ||
| # Stale state (renamed/removed exercises) must not inflate the count. | ||
| assert completed_count(["a", "b", "c"], {"a", "b", "ghost"}) == 2 | ||
|
|
||
|
|
||
| def test_completed_count_empty() -> None: | ||
| assert completed_count([], set()) == 0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from pythonlings.screens.welcome import welcome_text | ||
|
|
||
|
|
||
| def test_welcome_text_explains_the_loop() -> None: | ||
| text = welcome_text() | ||
|
|
||
| assert "I AM NOT DONE" in text | ||
| assert "save" in text.lower() | ||
| assert "F1" in text |
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.
Topic-only completion still renders a global “All exercises complete” header.
In the
elsebranch (not fully complete curriculum),show_final(...)is still used. That method hardcodes the global completion header inpythonlings/widgets/output_panel.py, so users can see a misleading completion state when only the current topic is done.Suggested direction
🤖 Prompt for AI Agents