search

Midnighter / structurizr-python

A Python 3 port of Simon Brown's Structurizr diagrams-as-code architecture description tool.
https://structurizr.com/
Apache License 2.0
65 stars 16 forks source link

Allow suppression of "archive" JSON file creation in `ClientSettings` #54

Closed yt-ms closed 3 years ago

yt-ms commented 3 years ago

Checklist

Is your feature related to a problem? Please describe it.

Currently, whenever you fetch a workspace from the Structurizr API, a file is created with the zipped JSON. If you don't desire this behaviour then you can work around it (e.g. by using tempfile.TemporaryDirectory() and putting that in the ClientSettings then cleaning it up afterwards) but this is a bit clumsy and runs the risk of leaving stray files/directories if the process is terminated prematurely.

Describe the solution you would like.

Currently ClientSettings defaults workspace_archive_location to Path.cwd() if unspecified. I propose that instead it should be made optional and if None then StructurizrClient.get_workspace() not generate an archive file, plus by default it is None so archive files do not get created.

Describe alternatives you considered

Alternatively, we could add an explicit boolean flag to ClientSettings to indicate whether archive files should be created, but this seems a little unpythonic.

Midnighter commented 3 years ago

I agree that one should be able to disable this. However, I have reservations against turning it off by default. When you have an existing workspace, it's quite easy to ruin it by uploading a faulty workspace. So I see the automatic backup as a feature to protect users from themselves. Additionally, I can't say for sure yet that the Python client can faithfully handle every workspace so it's even more important to save a version in my opinion.

I think setting the archive location to None is a fine solution but a Boolean flag on client settings would also be okay. Why does that seem unpythonic to you?

yt-ms commented 3 years ago

I agree that one should be able to disable this. However, I have reservations against turning it off by default. When you have an existing workspace, it's quite easy to ruin it by uploading a faulty workspace. So I see the automatic backup as a feature to protect users from themselves. Additionally, I can't say for sure yet that the Python client can faithfully handle every workspace so it's even more important to save a version in my opinion.

Yeah, that makes sense.

I think setting the archive location to None is a fine solution but a Boolean flag on client settings would also be okay. Why does that seem unpythonic to you?

It feels unpythonic as it introduces potential inconsistency (if the Boolean flag is False but the string is populated then what does that actually mean?) and somewhat redundant (None is a perfectly normal way to indicate lack of something rather than requiring a separate Boolean). But I'm happy to go whichever direction you'd prefer.

Midnighter commented 3 years ago

Great, I agree that setting workspace_archive_location to None is a clear signal.

If your main concern is cluttering the working directory, we could also create a structurizr directory with appdirs where the copies are stored by default. This is a bit more obscure since users will not necessarily be aware that this is happening but I think it would be very reasonable behaviour. We could even clean up files there that are older than some time interval.