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

Py2 ConfigParser doesn't raise errors on multiple same-named sections #143

Closed
untitaker opened this Issue Dec 13, 2014 · 18 comments

Comments

Projects
None yet
2 participants
@untitaker
Copy link

untitaker commented Dec 13, 2014

I can specify multiple sections named [Repository foo] and the values are merged in a way that is only obvious to people who know about the internals of Python 2's ConfigParser.

I think OfflineIMAP should work around http://bugs.python.org/issue2204, which is fixed in Python 3.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 9, 2015

Please, submit upstream.

@nicolas33 nicolas33 closed this Jan 9, 2015

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

This is fixed upstream.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

Ok, I might be missing of background. The thing is we (the maintainers) don't have the time to make our own home work.
I'm willing to re-open but please make it clear what is the situation and how OfflineIMAP is wrong in this context.

I mean, OfflinIMAP should NOT support multiple same-named sections because it's NOT a valid configuration we could expect for any section. Looks like with Py2 OfflineIMAP is right at stopping its job, to me.

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

Looks like with Py2 OfflineIMAP is right at stopping its job, to me.

It doesn't though. With "fixed upstream" I meant that this is fixed in Python 3. Python 2 has the same issue, but I am not sure whether fixing it is

  • considered critical enough to be included in Py2.
  • is worth the backwards compat break.
@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

FWIW I am happy to submit a PR regarding this, provided

  • you consider this worth the maintenance effort
  • I find a sensible workaround that doesn't involve monkeypatching. So far it doesn't seem promising.
@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

Again, Py2 OfflineIMAP is right at stopping its job, to me. I don't care much about the Py3 case for now because OfflineIMAP does not support Py3 today and we have so much to do. We are even late in merging contributions and pull requests.

So, unless you or another maintainer ( @konvpalto @xnox @spaetz or @X-Ryl669 ) wants to dig into this and write a patch I won't do the job myself. Here is my own position.

For merging a provided patch, I need to be conviced that the complexity AND the trouble of the patch worth the issue. For now, I think the current behaviour is correct and if I would do the job myself I would rather fix the Py3 OfflineIMAP behaviour which sounds wrong. I think we DON'T WANT to support multiple same-named sections whatever CustomConfig permits or not.

IOW, I'm not taking the upstream bug the same way because it doesn't fit our use case and the expectations we have regarding the configuration file.

Hope it makes sense. :-)

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

And thank you much for the time and interest you put into OfflineIMAP. :-)

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

I understand your stance on fixing this, but this line confuses me:

I think the current behaviour is correct

I strongly disagree, OfflineIMAP currently melds multiple sections into one, also overriding section keys silently.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

Ah, ok. This is the point I was missing while clearly stated in your initial comment. Sorry about that.
I re-open. I still won't consume my time to fix it myself because it does not sounds critical enough to me.

Of course, if someone comes with a correct patch, I'll take it.

Thanks for your insistence.

@nicolas33 nicolas33 reopened this Jan 10, 2015

@nicolas33 nicolas33 added the bug label Jan 10, 2015

@nicolas33 nicolas33 added this to the undefined milestone Jan 10, 2015

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

Thanks, and sorry for being not clear. I didn't know OfflineIMAP doesn't support Py3, so I could've left that out.

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

I have come up with this: pimutils/vdirsyncer#163

Is this worth it for you?

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

Honestly at this point I think it'd be more sensible to vendor a backport of Python 3 configparser module, or just close this as wontfix.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

I have come up with this: pimutils/vdirsyncer#163

That's what I feared. The fix is good and look very nice but I find that the added complexity (yet again playing with inheritance) for just this issue does not worth it, IMHO.

Honestly at this point I think it'd be more sensible to vendor a backport of Python 3 configparser module, or just close this as wontfix.

I fully agree. I know there was an attempt in the past to change of config parser library. I think it was in favour of the older brother configparser. The latest backported configparser looks to be a better choice today since it's obviously more Py3 aware. However, I'd be carefull about unicode legacy compatibility.

I'm working on unicode and the work is still under way. It's a subtle and hard work, BTW. So, you might want to wait for it if the backported configparser causes regressions. I didn't look much about the how unicode is handled in this module but you should expect subtle bugs due to unexpected encodings or variable types.

OTOH, I don't expect my current work to be actually the default in the short term. I hope to introduce it in the next release but I'll make OfflineIMAP unicode support optional first due to the subtle bugs I'm talking about.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Jan 10, 2015

Oh, and whatever you decide I'd rather to keep the issue open. It's a valid bug and I'd rather avoid someone else come with the same request and open new issues. With this one open, we have better chances to catch the analysis we made and prevent from doing the same work again and again.

@untitaker

This comment has been minimized.

Copy link

untitaker commented Jan 10, 2015

I agree.

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Mar 18, 2015

Will add that in the "known issue" section of the man page and close.

@nicolas33 nicolas33 self-assigned this Mar 18, 2015

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Mar 19, 2015

@nicolas33 nicolas33 removed this from the 6.5.7 (next minor) milestone Mar 19, 2015

@nicolas33 nicolas33 removed their assignment Mar 26, 2015

@nicolas33

This comment has been minimized.

Copy link
Member

nicolas33 commented Apr 7, 2015

Finally, I'm not fine with adding a dependency just because of this issue.

@nicolas33 nicolas33 closed this Apr 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment