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

Message board contrib #3525

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Message board contrib #3525

wants to merge 13 commits into from

Conversation

chiizujin
Copy link
Contributor

This is an inital version of a generic message board. It is used by creating an EvMessageBoard object in a room and using the board command to access it (see help board).

It supports these locks:
brdpost: - Controls posting to the board.
brdchange: - Controls changing (editing) or deleting your own posts.
brdmanage: - Allows changing and deletion of any post as well as clearing the board completely.

Before I create contrib-specific things, such as unit tests and a readme file, I'm submitting it as a draft for feedback/review on the following:

  • First of all, if this is something that would be accepted as a contrib.
  • Secondly, if there is anything here I should be doing differently.

There are a few points I'd like to cover:

  1. This is not yet thoroughly tested (and requires unit test, as mentioned).
  2. I'm duplicating some data in the board's dict because:
    • It allows listing of messages without hitting the database (Msg).
    • The original poster's name is retained even if the character is deleted.

This sort of makes the usage of Msg questionable, but using it means:

  • It's possible to retrieve messages that belong to a specific board, if required.
  • It allows for efficient deletion of all of a board's messages in the event of object deletion or board/clear
  1. It currently uses a version of utc_to_local from my page PR since all message times are currently in UTC, but I can remove this until that issue is resolved if required.
  2. The use of EvEditor (well, the saving/submitting part) feels a little clunky but I'm not sure of a better way to do it.
  3. I'm using the slightly less intuitive /change instead of /edit as /edit is the convention for invoking EvEditor.
  4. It currently uses a workaround for a non-auto-create dictionary attribute being lost over a restart ([BUG] AttributePropery with mutable, non-auto-create default loses its value on restart #3524).
  5. The object and command include 'Ev' to hopefully avoid conflicts.

And a few specific questions:

  1. Should the locks be mentioned in the help docstring or just in the readme?
  2. Should the command be changed to @board? The mail contrib uses @mail but the page command is just page. "board" feels like it may be easily shadowed if someone were to implement boats, for example.
  3. Should all of the functions have docstrings?

@InspectorCaracal
Copy link
Contributor

InspectorCaracal commented Apr 29, 2024

This is something we get asked for a lot! I think it'd be a great contrib; I'll pull this branch down and give it a test later.

Some thoughts on the points you outlined in your PR post here, first, though, without having actually looked at the code yet:

  • I don't think it's necessary to avoid database calls to read or post board messages. Unlike game mechanics, message board posts are generally going to be very lightly accessed, and the read overhead involved in doing a single SELECT query to retrieve a board's posts is going to be negligible enough that the potential gotchas and increased memory usage of caching them all in an in-memory dict (desyncs, etc) are, in my opinion, not really worth it.
  • Using EvEditor by default is a good idea, I think, but (if you haven't already done so) I'd strongly recommend having it be a pluggable option. Something that a game dev can easily override in their own game without having to reimplement the contrib.
  • I think it might be easier to read and if you use less specially-named locks: read, write, edit, manage. Usually the context of the locks are a product of the thing it's being used by. I don't think it's a big deal either way, though.

And your specific questions!

Should the locks be mentioned in the help docstring or just in the readme?

  • IMO they should be mentioned in the help text for any commands which allow you to set or change the locks. If this is only done in code, then no.

Should the command be changed to @board? The mail contrib uses @mail but the page command is just page. "board" feels like it may be easily shadowed if someone were to implement boats, for example.

  • Up to you. @board would probably be more widely usable as-is, but it's also extremely easy to change the key/aliases for a contrib command as a dev.

Should all of the functions have docstrings?

Since this is your first contrib, the guide on writing a contrib is also very handy! I discovered it long after my first contrib and wished I'd had it earlier 😂

@InspectorCaracal
Copy link
Contributor

Also, a couple quick examples of what kind of thing I mean by making it "a pluggable option":

  • In the clothing contrib, a lot of the mechanics such as clothing slot limits and what types of things can cover other things are defined as custom settings you can put in your settings.py file to override the contrib's default.
  • In my achievement contrib I made the achievements command's display output more easily customized by implementing it outside of the command func, so you can subclass the command class and override just the display without having to reimplement all of the command's logic.

@chiizujin
Copy link
Contributor Author

chiizujin commented Apr 30, 2024

I don't think it's necessary to avoid database calls to read or post board messages ...

That's fair enough then. I suppose I could store all of the other meta-data in the message as well now that I think about it. Though that would mean that the read_by data would have to be by character name rather than an object reference, which breaks if someone deletes a character and someone else creates one with the same name. Unless there's a way to attach meta-data to a Msg. Maybe I could move author_name into the Msg.message and leave read_by in the dict. Or maybe there's a way to retrieve the original sender's name after they have been deleted.

Using EvEditor by default is a good idea, I think, but (if you haven't already done so) I'd strongly recommend having it be a pluggable option ...

I haven't but I like that idea! I can do that for other parts of it as well - all of the presentation and the table object itself.

I think it might be easier to read and if you use less specially-named locks: read, write, edit, manage. Usually the context of the locks are a product of the thing it's being used by.

I had them that way in the beginning but renamed them to avoid possibly breaking something else. I'll change them back; I don't like the brd prefix to be honest.

IMO they should be mentioned in the help text for any commands which allow you to set or change the locks. If this is only done in code, then no.

It's just done through thelock command, so that's probably a no then.

Up to you. @board would probably be more widely usable as-is, but it's also extremely easy to change the key/aliases for a contrib command as a dev.

I think I'll change to @board.

Since this is your first contrib, the guide on writing a contrib is also very handy! I discovered it long after my first contrib and wished I'd had it earlier 😂

Luckily I found that first, though I haven't read it in detail yet. Unlike last week when I implemented the @open command because I'd forgotten about it, then found it when I was almost done.

Thanks for the feedback!

@InspectorCaracal
Copy link
Contributor

InspectorCaracal commented Apr 30, 2024

I don't think it's necessary to avoid database calls to read or post board messages ...

That's fair enough then. I suppose I could store all of the other meta-data in the message as well now that I think about it. Though that would mean that the read_by data would have to be by character name rather than an object reference, which breaks if someone deletes a character and someone else creates one with the same name. Unless there's a way to attach meta-data to a Msg. Maybe I could move author_name into the Msg.message and leave read_by in the dict. Or maybe there's a way to retrieve the original sender's name after they have been deleted.

Ah, that's a good point. The easiest way is probably to add a tag, like board_msg.tags.add(reader.id, category="read_by") (using the dbid, not name) but I haven't thought through it in detail to see if there would be any trip-ups in that approach.

@chiizujin
Copy link
Contributor Author

chiizujin commented Apr 30, 2024

Ah, that's a good point. The easiest way is probably to add a tag, like board_msg.tags.add(reader.id, category="read_by") (using the dbid, not name) but I haven't thought through it in detail to see if there would be any trip-ups in that approach.

Yes, tags would work well for read_by.

I also just found this in the Msg class:

# header could be used for meta-info about the message if your system needs
# it, or as a separate store for the mail subject line maybe.

So I think I'll putsubject in the header and author_name and read_by in tags, then that's all of the meta-data with the Msg.

I thought about doing something like author_name:subject in the header but that would break for anyone allowing the delimiter in character names. Maybe \n would work.

Edit: I have moved the message meta-data onto Msg pretty much as above.

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.

None yet

2 participants