BaseProfiler save() for json

[micdavis]

• Implemented _json_save_helper()
• implemented save() to support json
• wrote initial tests

[micdavis]

basically if the filepath doesn't end in "json" just default to pkl

[micdavis]

or if the filepath doesn't exist

[JGSweets]

We might want to use python's pathlib as it auto will get for us properly and handle strange cases like:

import pathlib


print(pathlib.Path("hello/foo.bar.tar.gz").suffix)
# output: ".gz"

 print(pathlib.Path("hello/foo.bar.tar.gz/a").suffix)
# output: ""

notably the last case which has . but not as the ext

[micdavis]

testing NotImplementedError()

[JGSweets]

Was this a needed add?

[taylorfturner]

removed

[micdavis]

testing save on StructuredProfiler() with csv file

[JGSweets]

Nice

[taylorfturner]

wonder if this should be a bit more clear... the filepath connotates to me just the folder path... not necessarily the name of the file ... but the filename is vital here with its extension becuause the core logic to testing wether to go with _pkl_save_helper or _json_save_helper is keying off the extension of the file

[taylorfturner]

we allow for None, yes; but the code in _pkl_save_helper auto creates a filepath (really it is create a filename) automatically with a timestamp for uniquness

[JGSweets]

I think the logic we might is more like:

1. use type first to determine how to save
   a. basically can force save with any extension to any type
2. if type=None look to extension to save
3. if extension is not clear, choose pickle
4. if filepath not specified, choose pickle

[taylorfturner]

use type first to determine how to save

agree

[taylorfturner]

I think there is a 🐛 (where the test shouldn't pass / we are missing assert / validation) and a couple questions / thoughts for improvement

[taylorfturner]

don't we need some kind of file name at least for this because with open("", "w") as f:, even if it did succeded wouldn't be ideal for finding the thing it wrote

[taylorfturner]

see suggestion above

[taylorfturner]

this is sneaky because it probably silently runs but doesn't do what we actually want ... 💀

[JGSweets]

good callout

[taylorfturner]

Suggested change

	filepath = ""
	if filepath is None:
	filepath = "profile-{}.json".format(
	datetime.now().strftime("%d-%b-%Y-%H:%M:%S.%f")
	)

[JGSweets]

good call, should be same as pkl technique.

[taylorfturner]

we allow for None, yes; but the code in _pkl_save_helper auto creates a filepath (really it is create a filename) automatically with a timestamp for uniquness

[taylorfturner]

remove

[taylorfturner]

Suggested change

self.maxDiff = None

[taylorfturner]

nice

[taylorfturner]

two thoughts (not hard and fast here), but we should validate the return_value from json.dumps() here... I say that because the error in the code above would have been caught. Maybe there is a return_value from json.dumps() that we could validate the filepath variable that is being actually written... we'd have to mock I reckon

[taylorfturner]

or just add a test validating passing None to the filepath parameter... and validate it outputs the autogenerate filepath (would have to mock the time generation)

[JGSweets]

if we decide to keep this format and we allow filepath to be None, this typing needs to have a | None

[JGSweets]

above in save we need to add the type: str | None with default None where we can choose "pickle "| "json"

[micdavis]

done

[JGSweets]

Here we need to add the type: str | None with default None where we can choose "pickle "| "json"

[JGSweets]

Fix extension here

[taylorfturner]

good catch

[JGSweets]

really should clarify any variable that is a mock as a mock.

e.g. mock_time_now

[JGSweets]

`"dataprofiler.profilers.profile_builder.datetime"

[JGSweets]

"dataprofiler.profilers.data_labeler_column_profile.DataLabelerColumn.update"

[JGSweets]

"dataprofiler.profilers.profile_builder.datetime"

comments resolved