example-scripts icon indicating copy to clipboard operation
example-scripts copied to clipboard

Add Example: File Saving and Loading

Open XertroV opened this issue 2 years ago • 7 comments

One todo question before PR is ready:

From line 213ish:

        // But if you use ::Write on a file that *already* exists then you risk deleting all the data in it!
        // todo: will it always delete the data or can you .Seek to only write certain bytes at certain offsets?
        IO::File csvFileInit(csvFilePath, IO::FileMode::Write);

I'm not sure how ::Write mode works, but I know enough to know there's potential danger.

Any suggestions on that part of the example?

XertroV avatar Jun 13 '22 04:06 XertroV

There is a lot of detail in this example, perhaps a little bit too much detail?

For example, CSV can be a useful format, but is it useful to understand the concept of CSV when reading an example that is about file IO? There are some Json-specific IO operations too which makes sense, but I'm not sure how in-depth you need to go with that. All this complexity ends up with a massive example script that conveys way too many ideas and examples. (For example, Main branching into 3 separate examples, including a yield() that is kind of unrelated to the topic at hand as well.)

I appreciate the work put into this though. Maybe it can be split up into multiple different example scripts. This repository was mostly made as a place to show short and concise examples, while this script is 300 lines long.

I also still want to implement a plugin-specific data storage which would probably also change this example.

codecat avatar Jun 13 '22 11:06 codecat

There is a lot of detail in this example, perhaps a little bit too much detail?

Sure. I agree it's on the detailed side. I don't think multiple files helps that, tho, since then there'd be repeated code (directories stuff) and require cross referencing or multiple files anyway.

There's a reason I used this much detail: Anyone doing anything substantial with IO will likely need to know more than this level of detail, and knowing less is not that useful. (That's IME)

I'll consider making changes depending on the answer to this q:

  • If a new dev came along and read this, would they be worse off?

IMO, whether the example is long or not isn't the main issue. The main issue is that the only way to get much of the info in this file is trial-and-error or looking thru other ppl's plugins. The latter is problematic sometimes b/c licensing.

The other big issue this solves IME is that there are details like how to actually open a file that are missing from the main docs.

This repository was mostly made as a place to show short and concise examples

FYI "short and concise" is not mentioned anywhere in the repository. The readme implies that comments and explanation are more important than conciseness.

XertroV avatar Jun 13 '22 22:06 XertroV

The other big issue this solves IME is that there are details like how to actually open a file that are missing from the main docs.

Right, but do we really need to explain the concept and how to write code to parse CSV? That seems a little out of scope for me.

codecat avatar Jun 14 '22 01:06 codecat

Right, but do we really need to explain the concept and how to write code to parse CSV? That seems a little out of scope for me.

No, that's fair enough. I'll trim it down and just mention that this sort of thing is useful for CSV stuff in a comment.

XertroV avatar Jun 14 '22 02:06 XertroV

Now that this issue is implemented (see comments on that thread for more info) this could potentially be revisited?

codecat avatar Jul 10 '22 19:07 codecat

Now that this issue is implemented (see comments on that thread for more info) this could potentially be revisited?

yup

XertroV avatar Jul 11 '22 07:07 XertroV

@codecat I think this is ready for review. It's much more streamlined now. I noticed https://github.com/openplanet-nl/issues/issues/161 while working on this, IDK if the folder-creation behavior (or lack of) is intended or not, but the script assumes that FromDataStorage is working as intended atm.

XertroV avatar Jul 24 '22 01:07 XertroV