Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Example: File Saving and Loading #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

XertroV
Copy link
Contributor

@XertroV XertroV commented Jun 13, 2022

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?

@codecat
Copy link
Member

codecat commented Jun 13, 2022

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.

@XertroV
Copy link
Contributor Author

XertroV commented Jun 13, 2022

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.

@codecat
Copy link
Member

codecat commented Jun 14, 2022

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.

@XertroV
Copy link
Contributor Author

XertroV commented Jun 14, 2022

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.

@codecat
Copy link
Member

codecat commented Jul 10, 2022

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

@XertroV
Copy link
Contributor Author

XertroV commented Jul 11, 2022

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

yup

@XertroV
Copy link
Contributor Author

XertroV commented Jul 24, 2022

@codecat I think this is ready for review. It's much more streamlined now.
I noticed openplanet-nl/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants