-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
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
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
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
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.
Now that this issue is implemented (see comments on that thread for more info) this could potentially be revisited?
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
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.
One todo question before PR is ready:
From line 213ish:
I'm not sure how
::Writemode works, but I know enough to know there's potential danger.Any suggestions on that part of the example?