shadoxxhd / steamworkshopdownloader

download steam workshop items via steamcmd
317 stars 50 forks source link

logging module as an output method instead of prints #24

Open cartaplassa opened 2 years ago

shadoxxhd commented 2 years ago
  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 commented 2 years ago
  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 commented 2 years ago
  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 commented 2 years ago

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 commented 2 years ago

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