allow Paths in save() by RoyalTS · Pull Request #83 · altair-viz/altair_saver

RoyalTS · 2020-11-30T20:46:02Z

RoyalTS · 2020-11-30T20:54:56Z

Not quite sure where the test failures are coming from. They seem unrelated to the changes I made

jakevdp · 2020-12-01T00:32:17Z

Looks good! We'll need some tests of this here as well. I think you could add one of path-based format inference here:

altair_saver/altair_saver/tests/test_core.py

Lines 309 to 315 in e710a76

    
           def test_infer_format(spec: JSONDict) -> None: 
        
               with temporary_filename(suffix=".html") as filename: 
        
                   with open(filename, "w") as fp: 
        
                       save(spec, fp) 
        
                   with open(filename, "r") as fp: 
        
                       html = fp.read() 
        
               assert html.strip().startswith("<!DOCTYPE html>")

and add one of file opening and writing here:

altair_saver/altair_saver/tests/test_utils.py

Lines 63 to 72 in 46cdbff

    
           @pytest.mark.parametrize("mode", ["w", "wb"]) 
        
           def test_maybe_open_filename(mode: str) -> None: 
        
               content_raw = "testing maybe_open with filename\n" 
        
               content = content_raw.encode() if "b" in mode else content_raw 
        
               with temporary_filename() as filename: 
        
                   with maybe_open(filename, mode) as f: 
        
                       f.write(content) 
        
                   with open(filename, "rb" if "b" in mode else "r") as f: 
        
                       assert f.read() == content

As a side-note, it looks like we don't have any integration tests of saving a chart to a disk. That is indirectly covered by unit tests of the relevant utilities, but since saving a chart is a core functionality of this library, it would be better to have a full end-to-end test of that behavior!

RoyalTS · 2020-12-01T14:19:38Z

We'll need some tests of this here as well. I think you could add one of path-based format inference here:

Wouldn't

altair_saver/altair_saver/tests/test_utils.py

Line 34 in 46cdbff

@pytest.mark.parametrize(

be the better place for testing format inference from the passed fp?

If so, I'm not sure I understand the current test. The use_filename==False mode just seems like a more convoluted way of generating a filename, one that introduces a failure mode (the temp file not being created) that has nothing to do with the test. What am I missing?

jakevdp · 2020-12-01T14:38:43Z

The test aims to exercise format inference from either a file pointer (in which case fp.name is the relevant name) or a filename (in which the string itself is the relevant name). I'm not certain I understand what failure mode you're referring to.

RoyalTS · 2020-12-01T14:44:01Z

Oh, I see. In that case one would want to test four different possible values for fp?

string path
pathlib.Path
file pointer
output streams

with format inference working as inteded for 1-3 and failing in the fourth case?

jakevdp · 2020-12-01T14:47:56Z

Yeah, that sounds right.

jakevdp · 2020-12-01T17:34:33Z

New tests look great!

RoyalTS · 2020-12-02T19:09:02Z

The build seems to have encountered an internal error. Any way to trigger a new one manually?

jakevdp · 2020-12-02T19:27:32Z

I triggered a new run

RoyalTS · 2020-12-02T19:31:46Z

Lots of failing tests but none look like they're due to the changes?

jakevdp · 2020-12-02T20:47:32Z

Yeah, looks like there are failures on master due to tests not being resilient to dependency updates.

jakevdp · 2020-12-02T20:47:58Z

Also the lint job is just failing for reasons I don't understand.

RoyalTS · 2020-12-02T20:50:22Z

That makes two of us then :)

joelostblom · 2024-03-30T18:15:01Z

Thanks for you PR @RoyalTS . As you have noticed, there hasn't been any development in this repo for a while. Since Altair 5.2, the functionality of Altair Saver is now available in Altair via the vl-convert package. Most of the functionality has been available since 5.0, and the main addition in 5.2 was PDF export. See the docs on how to save charts for more details.

We are going to archive this repo, so I'm closing all the open issues and PRs before doing so. Try out the new options for saving charts mentioned above and if you run into issues, please open an issue directly in the altair or vl-convert repo.

allow Paths in save()

35bd7b2

RoyalTS force-pushed the save_paths branch from 6923a5c to 35bd7b2 Compare November 30, 2020 20:48

black formatting fixes

0b7558b

RoyalTS added 3 commits December 1, 2020 16:07

improve and consolidate tests for format inference

486bf7f

test maybe_open on paths

aad700a

black fixes

56e29b8

mypy fixes

9844c23

joelostblom closed this Mar 30, 2024

Conversation

RoyalTS commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoyalTS commented Nov 30, 2020

Uh oh!

jakevdp commented Dec 1, 2020

Uh oh!

RoyalTS commented Dec 1, 2020

Uh oh!

jakevdp commented Dec 1, 2020

Uh oh!

RoyalTS commented Dec 1, 2020

Uh oh!

jakevdp commented Dec 1, 2020

Uh oh!

jakevdp commented Dec 1, 2020

Uh oh!

RoyalTS commented Dec 2, 2020

Uh oh!

jakevdp commented Dec 2, 2020

Uh oh!

RoyalTS commented Dec 2, 2020

Uh oh!

jakevdp commented Dec 2, 2020

Uh oh!

jakevdp commented Dec 2, 2020

Uh oh!

RoyalTS commented Dec 2, 2020

Uh oh!

joelostblom commented Mar 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RoyalTS commented Nov 30, 2020 •

edited

Loading