Skip to content

Dlvax #2

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

Open
wants to merge 22 commits into
base: share_loc_websocket
Choose a base branch
from
Open

Dlvax #2

wants to merge 22 commits into from

Conversation

ewjax
Copy link

@ewjax ewjax commented Aug 1, 2022

No description provided.

@ewjax
Copy link
Author

ewjax commented Aug 1, 2022

I think it is working, and the need for the unused entries in the .json file is removed. The existing ParserWindow base class is broken up into a Parser base class, from which ParserWindow derives, and contains the virtual functions necessary to function with the calling nParse code. The maps, spells, and discord parsers continue to derive from ParserWindow as before, but the new DeathLoopVaccine parser derives from the base Parser class.

Copy link
Owner

@rm-you rm-you left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick first glance, don't have time to dig through this in super detail yet



class ParserWindow(QFrame):
class Parser:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence about this, but it's growing on me. Maybe there would be other things that don't need a window? IDK, still thinking

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does solve the problem neatly for any non-window parser, now or in the future.

# player deaths trigger the simulated process kill, or after any simulated
# player death events "scroll off" the death loop monitoring window.
#
class DeathLoopVaccine(Parser):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is cute but we may want to rename this something clearer like "DeathLoopPrevention", IDK.
Vaccines are apparently political now in 2022 or something XD

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you, I'm ambivalent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's your thoughts here? If you wish to change it, I'll bang it out with some other changes I'm in the middle of doing.

line = f'[{timestamp.strftime("%a %b %d %H:%M:%S %Y")}] ' + text

# cut off the leading date-time stamp info
trunc_line = line[27:]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from text?
IDK it feels like you likely could just rework the rest of the function to use the timestamp rather than having to reconstruct this other line, too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Two reasons:

  1. I did spend a fair amount of time getting them working for the standalone DLV program, which was written assuming the full, unchanged EQ log line was passed as input, and then the functions disect it into a timestamp and the rest of the line. It's time consuming and error prone to rewrite the whole rest of the function for no real utility gain.
  2. The bigger reason = the utility keeps a queue of time-stamped death messages, and then uses the time stamp portion of each death line to scroll them off when they get too old. Having a separate timestamp and rest of the line means I would have to rewrite it to completely change the logic somehow, either with having two containers, or only keeping the timestamps but not the death message, or something. Again, a bunch of error prone work for no real utility gain.

So I just took the passed data, put it back together, and then copied my already-developed functions in unchanged.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd like to take a crack at doing it a slightly different way that doesn't require this kind of text mangling. Date-time item expiration is a pretty common pattern and I do it already in some other places.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I did rework it some to make use of the passed (timestamp, text) data where possible, so it's now at least a bit more sensible than mindless combination immediately followed by separation, which is what it was doing before. The current logic does however keep the bit about recombining them for retention, expiration, and reporting purposes, so if you want to rework that bit for consistency you can.


# now purge any death messages that are too old
done = False
while not done:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still just scanning, but at first glance this doesn't look super Pythonic and I'm guessing this loop can be reformatted

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

# reconstruct the full logfile line
# this is a bit counter-intuitive, but the rest of the logic in this function was
# developed assuming the line was the full line, including the time stamp
line = f'[{timestamp.strftime("%a %b %d %H:%M:%S %Y")}] ' + text
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not totally following why this is necessary to do, if you have a timestamp you can just use it, there's only one line involved here and it looks like half the time you're truncating this stuff off anyway

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

ewjax and others added 15 commits August 1, 2022 17:53
1. char_name moved out of .json and into a global variable.
2. standalone functions move into helpers
1. deathloop.deaths and deathloop.seconds are initialized at every use, not just at instantiation
2. cleaned up processing of (timestamp, text) parameters in parsing functions
3. reverted version number to 0.6.4
4. cleaned up some additional empty functions in ParserWindow
5. cleaned up global.char_name usage
Not totally sure what this being broken actually caused...
Sharing will stop correctly in all cases now.
This seems to be working correctly internally to PyInstaller as of the
newer 5.x releases.
Honestly this doesn't matter, as it isn't threaded, but it looks
cleaner than using "global" statements.
Add loading splash screen, clean up some bugs
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