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

Expose NoSectionError at package level #2517

Closed
wants to merge 2 commits into from
Closed

Expose NoSectionError at package level #2517

wants to merge 2 commits into from

Conversation

honnix
Copy link
Member

@honnix honnix commented Sep 6, 2018

Description

Expose NoSectionError at package level so that from luigi.configuration import NoSectionError would work.

Motivation and Context

It used to work like this until #2457, so we should probably bring it back for backward compatibility.

Have you tested this? If so, how?

TBD

@ulzha
Copy link
Contributor

ulzha commented Sep 6, 2018

+1

I guess the reason we "should probably" is just so that we keep compatibility?

The whole idea of using NoSectionError exception as a way for checking if a section exists, seems like abuse of exceptions.

@honnix
Copy link
Member Author

honnix commented Sep 6, 2018

Yeah the reason is to keep backward compatibility. It is indeed a bad idea to abuse exception. Maybe we should instead let it break in this case.

@honnix
Copy link
Member Author

honnix commented Sep 6, 2018

@Tarrasch @dlstadther wdyt?

@dlstadther
Copy link
Collaborator

Am i correct that NoSectionError is just coming from configparser/ConfigParser? Other places in Luigi which need that just reimport from config parser. So i'd say no need to keep in package init

@honnix
Copy link
Member Author

honnix commented Sep 6, 2018

You are right it is from ConfigParse. The problem is not Luigi itself but user code because they could access from init, and now that breaks. The risk is low and very easy to fix by users themselves if we ever get complaints.

@dlstadther
Copy link
Collaborator

Yeah, i'm fine letting that break. At some point, we need to drop support for certain things. This is a small case for that.

@honnix
Copy link
Member Author

honnix commented Sep 6, 2018

👍 Let's go for it.

@honnix honnix closed this Sep 6, 2018
@honnix honnix deleted the honnix-patch-1 branch September 6, 2018 12:42
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.

3 participants