Add functionality to read files instead of raw strings #35

konnerhorton · 2025-05-08T18:37:55Z

Fixes issue #6 by adding to bedrock_ge.ags.read

Add read_ags_source to read filepath or file-like objects (returns a context manager stream
Add detect_encoding to infer encoding from object (user can provide encoding)
use the two functions above in ags_to_dfs to read the source

Add test for detect_encoding and ags3 sample data.

Add `detect_encoding` to be called in `ags_to_dfs` Add `read_ags_source` to be called in `ags_to_dfs` ( Update `ags3_to_dfs` for same Update `ags4_to_dfs` for same Add docstring to `coerce_string`

`test_detect_encoding` was also testing strings containing ags data (which is not needed), causing github workflows to fail.

JoostGevaert · 2025-05-13T09:02:13Z

Thanks for this amazing PR, Konner!
I'm working on it, but realized that I actually also need to make an update to the automated tests.

src/bedrock_ge/gi/ags/read.py

Update docstrings to remove relic args Update docstrings for typos

JoostGevaert · 2025-05-19T07:52:36Z

Awesome, I think I'm done with the review. Now I need to redo the automated test on the Kai Tak Hong Kong example marimo notebook a bit, then we can see if that notebook needs any changing due to the new way of handling files that you implemented, and then this PR can be merged.

JoostGevaert · 2025-05-23T10:12:53Z

Hi @konnerhorton, we've updated the test that runs the Kai Tak, HK example marimo notebook in the dev branch. Could you please merge the dev branch into your branch and then push that merge commit to make sure we're running that updated test on the code you are contributing?

konnerhorton · 2025-05-24T08:49:25Z

Done, looks like the automated tests passed.

JoostGevaert · 2025-05-26T08:08:12Z

Oke, then still something isn't going quite right with the tests, because the notebook shouldn't be able to run anymore, because previously the notebook was performing the AGS 3 data to dataframes operation on a string, and now that shouldn't be possible anymore.

Nonetheless, I'll merge this PR and have a look at what is going on here exactly in a different PR myself.

Thanks a million for the fantastic work @konnerhorton!

JoostGevaert · 2025-05-26T08:21:30Z

Ah, I think that the problem is that I'm still running the marimo notebook with uv run rather than python, which still causes uv to create a new virtual env with the latest version of bedrock-ge from PyPI insteaed of the local code, so I actually still didn't solve that issue with my latest PR into dev.
Just FYI :)

konnerhorton added 4 commits May 8, 2025 13:34

Update ags_to_dfs to accept file path instead of ags_data as string

80a4b4f

Add `detect_encoding` to be called in `ags_to_dfs` Add `read_ags_source` to be called in `ags_to_dfs` ( Update `ags3_to_dfs` for same Update `ags4_to_dfs` for same Add docstring to `coerce_string`

Merge remote-tracking branch 'upstream/dev' into read-files

7ebc359

Update test_ags.py for new directory structure

1f1ea8c

Remove data strings from tests

82ef27c

`test_detect_encoding` was also testing strings containing ags data (which is not needed), causing github workflows to fail.

JoostGevaert reviewed May 14, 2025

View reviewed changes