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

logging module as an output method instead of prints #24

Open
wants to merge 13 commits into
base: experimental
Choose a base branch
from

Conversation

cartaplassa
Copy link

No description provided.

@shadoxxhd
Copy link
Owner

  1. line 157: that match is not an error condition; the "redirecting ..." line is appended to the last output line on windows (in a normal console it shows up at the beginning of steamcmd output, but process.stdout sees it only at the end), and the re.search is just visual cleanup
  2. line 159: the break there is probably unnecessary - it is a sufficient condition to check for end of the output, but the return_code condition should also be sufficient on its own.
  3. line 170: this should be .info, not .debug, since this is the last output of the program if for some reason it wrote something and returned since the last loop
  4. line 192: 3 lines of output for each item could be annoying when downloading large collections. Full paths should only be printed once per download order (or at most once per batch)
  5. line 193: is this guaranteed to be the same as join(expanduser(...),...)?
  6. line 195: the os.path.join(...,str(wid)) is necessary to prevent the files of different items from overwriting each other (in the case of eg. stellaris, that would occur any time more than 1 mod is downloaded)
  7. line 196: could the logger put this in the same line as the "moving " line? IMO the workshop IDs should be directly below each other.
  8. line 31: does the text.configure(state='disabled') still allow selecting & copying error messages?

@cartaplassa
Copy link
Author

  1. Yep, changed to logger.info
  2. I thought it's used to break the cycle if an error was found, but in my case steamcmd throws error CAppInfoCacheReadFromDiskThread took 266 milliseconds to initialize every time it's called. It isn't critical and I suspect it shouldn't even be called an error.
  3. Right, changed to logger.info
  4. In this case there's an option to just increase LOG_LEVEL to logging.INFO for public release, as DEBUG level is most often used in development only. That's how logging library makes the work easier - stuff you usually have to comment out before release can be just assigned to DEBUG level and made invisible with a single change of constant. Although, these lines have already outlived their purpose.
  5. Yes, the only difference is that if expandpath is top-level operation, it's easier to rewrite the insides of join.
  6. I don't get it. When I tested it, SWD used to make directories like "Rimworld/Mods/<WID>/<WID>".
  7. Is that how it should look like?
    image
  8. Yes, it only prevents user input.

@shadoxxhd
Copy link
Owner

  1. Then that seems to be a difference between platforms. On windows, it moves to Rimworld/mods/WID with the join(...,str(wid)), and directly to Rimworld/mods without it.
  2. Yes, except that I prefer "... DONE" over "... Item moved". Duplication of the task description in the same line seems unnecessary.

@cartaplassa
Copy link
Author

Sorry, that were two really busy weeks.

Problem 6 fixed with platform if-else check block, should suffice.
About 7 I had a problem with printing DONE before the thing is actually done, but ok.
That's how it looks like with line break:
Screenshot_20220803_193336
And without:
Screenshot_20220803_193230

@cartaplassa cartaplassa closed this Aug 3, 2022
@cartaplassa cartaplassa reopened this Aug 3, 2022
@cartaplassa
Copy link
Author

Those lines with numbers are gone, btw, I just needed them to check if it got the lines' lenghts right.

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