Iridescent Ivies #17
Iridescent Ivies #17
Conversation
* feat: basic stuff * feat: added a button for onclick and a text input * lint: tidy up for checks * feat: example for pyfetch on the backend.
Setup basic environment
based on @mimic version
Added clean ui implementation with boot sequence and visual effects. References setup.py for pyodide integration. Closes #4
and start of bsky api layer issue #6
Ensured pyodide, setup.py, functions.py, and frontend.py load asynchronously during boot sequence.
Add a parser for SQL
feat: select * from tables
Parse LIMIT clauses
Start showing query parse errors
fix: easter bug
lemonyte
left a comment
lemonyte
left a comment
There was a problem hiding this comment.
Congratulations on making it into the top 10 projects this year!
Although the jam is over, the comments I leave here and in the code review are intended as "constructive criticism" for your future improvement and reflection. While many of them point out issues, the project overall is very good considering you had just 10 days to make it.
You're encouraged to reply to any of the comments and questions, give me feedback, and discuss!
Commit quality
It's great to see lots of pull requests, especially with reviews and feedback!
The quality of commit messages was overall satisfactory, and I noticed they got a lot less organized in the last two days, but that's normal for a code jam.
Some commits were quite large, and there was a lot of "lint fixing" commits that could be avoided with a working pre-commit setup.
Code quality
I'm impressed with the code quality throughout the project, especially in the SQL parser and tokenizer implementation.
However, the front-end parts have quite a few JavaScript-y patterns and issues like reassigning global variables and a lot of possible None values. After translating from JavaScript to Python, I would go through and reorganize it a bit to make it more Pythonic, e.g. by using a class instead of global variables, by adding some checks for None, etc.
The formatting is great, it's always nice to have a tool automate the boring-yet-necessary work and free up your time. I see you've made a good effort to follow Ruff's linting rules, with some occasional noqas. However, the typing could really use some more attention.
Structure
Separating the core, BlueSky API, and the Python front-end code into separate packages was a good choice and made it easier to review. The HTML is simple and straightforward.
One thing I found somewhat confusing was how exactly the Python code was initialized and hooked into the HTML, as it looks like event handlers are registered in multiple places and the load sequence was unclear.
Typing
There are quite a few places in the code where type hints are either incorrect or there are no type guards in place. I like to use Pyright, at least in "basic" mode, to help me make sure I'm writing code that works with the types I'm receiving or returning.
Although this is understandably in part due to the code interfacing with JavaScript APIs that can have pretty vague or generic types, I think the project would have benefited from a type check action in the linting workflow. Not necessarily as a pre-commit hook, which I find slows down the development process, but as a GitHub-side check after commits and especially before merging pull requests.
Security
Because this project handles real login credentials, I decided to add a couple notes on security here.
It's good that the credentials are never saved persistently, but they are saved in an exposed object during runtime. This doesn't pose an immediate security risk, but it does open up the possibility of leaking the password through a cross-site scripting attack or through anything else that has access to the JavaScript context on the page.
console.log(window.session.password);I couldn't find any uses of the password after the initial login, so it would be a good idea to remove it everywhere except as a parameter to the login function, where it's only used in the auth request and not saved.
Documentation
Most of the functions have docstrings, and most of them are helpful and descriptive. Some are a little vague in parameters or return values.
The README is to-the-point and concise, but I would have liked to see some more detailed examples. For example, how can one query posts but show just the author's display name and post content? What are some of the more useful columns? There are quite a few columns containing long strings that make it a little hard to scroll through the data horizontally.
It would also be good to mention roughly what each team member contributed.
The setup instructions were super easy to follow which is always great for whoever is reviewing or demoing your project :)
Final notes
The visual style was probably the most impressive part of the project. I really liked the 80's CRT effects and coherent design. The real-time status display is a nice touch too.
The idea of an SQL interface to consume social media content on BlueSky fits the theme of "wrong tool for the job" really well.
Although the SQL input has some minor bugs, the core functionality is solid and you've even gone above and beyond by adding some tests for the tokenizer and parser, which is always impressive to see in a code jam.
I hope my review gives you some useful takeaways you can apply to other projects. There is always room to grow, but I saw a lot of things in this project that I liked!
-- lemonyte
| session_info: FetchResponse = await self.client.post( | ||
| endpoint, | ||
| headers={"Content-Type": "application/json"}, | ||
| data={ | ||
| "identifier": self.username, | ||
| "password": self.password, | ||
| }, | ||
| ) | ||
| session_info: dict = await session_info.json() |
There was a problem hiding this comment.
I would use separate names for the response and json data, to avoid confusing the type checker.
| # Refresh token | ||
| self.refresh_jwt = None | ||
|
|
||
| async def login(self) -> None: |
There was a problem hiding this comment.
| async def login(self) -> None: | |
| async def login(self) -> bool: |
| async def get_profile(self, actor: str) -> dict: | ||
| """Get a user profile.""" | ||
| # If no actor specified and we're authenticated, use our handle | ||
| if actor is None: |
There was a problem hiding this comment.
According to the type hint this code will never be run. If actor could be None, better to type as str | None in the parameter.
|
|
||
| Args: | ||
| q (str): the given query | ||
| sort (Literal["top", "latest"], optional): The sort Order. Defaults to "latest". |
There was a problem hiding this comment.
A regular ' char works instead of ", at least in VS Code. However, since the types are already expressed in the parameters its not really necessary to duplicate them in the docstring.
| async def search_posts( # noqa: PLR0913 | ||
| self, | ||
| q: str, | ||
| limit: int = LIMIT, | ||
| sort: Literal["top", "latest"] = "latest", | ||
| since: str = "", | ||
| until: str = "", | ||
| mentions: str = "", | ||
| author: str = "", | ||
| tag: str = "", | ||
| cursor: str = "", | ||
| ) -> dict: |
There was a problem hiding this comment.
Would be nice to use positional-only and keyword-only parameters, especially when theres a lot of them. Also applies to the other methods here.
| async def search_posts( # noqa: PLR0913 | |
| self, | |
| q: str, | |
| limit: int = LIMIT, | |
| sort: Literal["top", "latest"] = "latest", | |
| since: str = "", | |
| until: str = "", | |
| mentions: str = "", | |
| author: str = "", | |
| tag: str = "", | |
| cursor: str = "", | |
| ) -> dict: | |
| async def search_posts( # noqa: PLR0913 | |
| self, | |
| q: str, | |
| /, | |
| *, | |
| limit: int = LIMIT, | |
| sort: Literal["top", "latest"] = "latest", | |
| since: str = "", | |
| until: str = "", | |
| mentions: str = "", | |
| author: str = "", | |
| tag: str = "", | |
| cursor: str = "", | |
| ) -> dict: |
| } | ||
|
|
||
| // styles for boot screen (inject into document head) | ||
| const bootStyles = ` |
There was a problem hiding this comment.
These could probably be moved to styles.css
| mainInterface.style.opacity = "1"; | ||
| } | ||
|
|
||
| // setup fallback functionality |
There was a problem hiding this comment.
Nice to have a visible feedback when something goes wrong
| [dependency-groups] | ||
| dev = [ | ||
| "pre-commit~=4.2.0", | ||
| "ruff~=0.12.2", | ||
| "pytest", |
There was a problem hiding this comment.
It's good to include these here for type checking in the IDE.
| [dependency-groups] | |
| dev = [ | |
| "pre-commit~=4.2.0", | |
| "ruff~=0.12.2", | |
| "pytest", | |
| [dependency-groups] | |
| dev = [ | |
| "pre-commit~=4.2.0", | |
| "ruff~=0.12.2", | |
| "pytest", | |
| "pyodide-py~=0.27.7", | |
| "webtypy~=0.1.7", | |
| "micropip~=0.9.0", |
| if char == "": | ||
| break | ||
|
|
||
| if char in string.ascii_letters: |
There was a problem hiding this comment.
This would be a good place to use a match-case block.
|
|
||
|
|
||
| def _extract_images_from_post(data: dict) -> str: | ||
| """Extract any embedded images from a post and return them as a delimited string.""" |
There was a problem hiding this comment.
Also important to mention what the string is delimited by.
No description provided.