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

fix logging attribute error #226

Merged
merged 5 commits into from
Jul 2, 2020
Merged

Conversation

tlambert03
Copy link
Contributor

one possible solution that fixes #225 but still lets you log from self.ctx if you want. Or just let me know if you want to ditch self.ctx altogether.

@joshmoore
Copy link
Member

I'd err on the side of ditching. This class should really have no concept of self.ctx (That's a purely CLI thing.) Looking at this, I think this should also be a warn. The user has requested a timeout, and if that can't be set (or is incorrect) it seems like at least a WARN. (Perhaps the exception should even be thrown, but I'll need to review more for that.)

@tlambert03
Copy link
Contributor Author

k, happy to convert to a warn or raise. just let me know.

@joshmoore
Copy link
Member

Let's go with a warn and get it released, then I can follow up with the rest.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 26, 2020

k, do you want the formatted traceback in the warning?

EDIT: updated with a simple warning. let me know if you want specific info in there

@joshmoore
Copy link
Member

Sorry for being a pain, @tlambert03, but can we go back to 2d34519 and:

  • keep the CmdError / Exception split,
  • use logger.warn for the CmdError at WARN, and
  • use logger.error for the catch-all Exception?

@tlambert03
Copy link
Contributor Author

ok have another look at your convenience. if you also wanted the self.ctx check added back, lemme know.

@joshmoore
Copy link
Member

ok have another look at your convenience.

Awesome, thank you!

if you also wanted the self.ctx check added back, lemme know.

Nah. That should have never happened. 👍

@joshmoore joshmoore merged commit 11ae6e3 into ome:master Jul 2, 2020
@manics manics mentioned this pull request Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential AttributeError in SessionsStore.create
2 participants