msg312608 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-02-23 01:29 |
``ConfigParser.write()`` writes a superfluous final blank line.
Example::
import configparser
cp = configparser.ConfigParser()
cp['section1'] = {'key': 'value'}
cp['section2'] = {'key': 'value'}
with open('configparser.ini', 'w') as f:
cp.write(f)
The output file 'configparser.ini' will be:: (I added line number)
1 [section1]
2 key = value
3
4 [section2]
5 key = value
6
with a superfluous final blank line.
Compare to ``GLib.KeyFile``::
import gi
gi.require_version('GLib', '2.0')
from gi.repository import GLib
kf = GLib.KeyFile()
kf.set_string('section1', 'key', 'value')
kf.set_string('section2', 'key', 'value')
kf.save_to_file('glib.ini')
The output file 'glib.ini' will be:: (I added line number)
1 [section1]
2 key=value
3
4 [section2]
5 key=value
without a superfluous final blank line.
|
msg312615 - (view) |
Author: Steven D'Aprano (steven.daprano) * |
Date: 2018-02-23 04:50 |
Its not a superfluous blank line. It is standard convention for Unix tools to end text files (of which ini files are a kind of text file) with a final newline \n. There are various reasons for this, but it doesn't matter what those reasons are, the important thing is that it is (or at least, ought to be) intentional for the ini file to end with a \n.
It is not an error for the file to end with one (or more) blank lines, regardless of the platform; it is more convenient for all lines to a \n delimiter, rather than making the last line special.
I do not believe there is any good reason to complicate the code and the API with an unnecessary "trim_final_blankline" parameter, but even if there is a good reason, the default would have to be False to remain backwards compatible.
Unless you have a good reason why it should be considered an error for the INI file to end in a blank, this patch should be rejected, and instead we should have an explicit test to ensure that the INI file is always written with a final blank.
(Of course, when *reading* INI files, it should accept them either with or without final blank.)
|
msg312616 - (view) |
Author: Steven D'Aprano (steven.daprano) * |
Date: 2018-02-23 04:54 |
Oh, I forgot... even if it is decided that this trim_final_blankline parameter was desirable, the patch isn't sufficient to be accepted. You would need to also supply documentation and tests.
|
msg312619 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-02-23 06:34 |
**It's not final newline \n !**
Maybe what I said confused. The "final blank line"
does not mean the "final newline char".
It means an extra blank line after last line.
It might be clearer to represent it in a string::
'[section1]\nkey = value\n\n[section2]\nkey = value\n\n'
It has *two* \n at end. I think it should be only one.
|
msg312622 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-02-23 09:22 |
Patch updated, included documentation and tests.
|
msg313244 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-05 12:03 |
Adding two new parameters to control so tiny detail of the output looks excessive to me. What if just change the default behavior and never output a final blank line?
|
msg313480 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-03-09 10:39 |
I’m afraid of breaking the backward compatibility.
|
msg313513 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-03-10 06:37 |
If we treat the original behavior as a bug,
it’s much easier to write a patch
that just changes the default behavior
and never outputs a final blank line.
I have looked through the testsuite of it.
It seems that the original author had knew
the final blank line and treat it as
expect result. I’m not sure about it.
|
msg313518 - (view) |
Author: Steven D'Aprano (steven.daprano) * |
Date: 2018-03-10 10:27 |
On Sat, Mar 10, 2018 at 06:37:28AM +0000, TitanSnow wrote:
>
> TitanSnow <tttnns1024@gmail.com> added the comment:
>
> If we treat the original behavior as a bug,
> it’s much easier to write a patch
> that just changes the default behavior
> and never outputs a final blank line.
Remind me, why do we care about that extra blank line?
> I have looked through the testsuite of it.
> It seems that the original author had knew
> the final blank line and treat it as
> expect result. I’m not sure about it.
Is there a test for it? If so, then it should stay unless the author
agrees it can go, or we get approval on the Python-Dev mailing list.
|
msg313520 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-10 10:36 |
I thought a blank line between sections is just for readability, and no program will break if drop it. Removing the final blank line can harm the readability in the case of sequential writes to the same file (unless we detect the start of the file, that is problematic in the case of non-seekable streams). But I didn't thought that a superfluous final blank line causes any problems. What software has a problem with it?
|
msg313521 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-03-10 11:32 |
> But I didn't thought that a superfluous final blank line causes any problems. What software has a problem with it?
Currently I have not found a program that has problem with
the superfluous final blank line, and I think there won’t be.
> Removing the final blank line can harm the readability in the case of sequential writes to the same file
Sorry that I have not thought about this.
In this way, the prev patch that adds two new parameters might be better.
> Remind me, why do we care about that extra blank line?
Well, though it seems that that extra blank line won’t cause any problem,
it does not look nice I think, and is different with INI files written
by other applications.
|
msg313525 - (view) |
Author: TitanSnow (tttnns) * |
Date: 2018-03-10 12:57 |
For the case of sequential writes to the same file,
I think it’s a invalid use case.
The file can be created or modified by
user or other applications, breaking the assume of
ConfigParser. It’s better to have a method to merge
two ConfigParser objects then writes it into the file
at one time.
|
msg313530 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-10 15:05 |
If there is no real problem, I think it is not worth to change this.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:58 | admin | set | github: 77098 |
2021-12-09 09:39:01 | iritkatriel | set | status: pending -> closed stage: patch review -> resolved |
2021-10-03 01:50:44 | martin.panter | link | issue45349 superseder |
2018-03-10 15:05:50 | serhiy.storchaka | set | status: open -> pending resolution: rejected messages:
+ msg313530
|
2018-03-10 12:57:50 | tttnns | set | messages:
+ msg313525 |
2018-03-10 11:32:41 | tttnns | set | messages:
+ msg313521 |
2018-03-10 10:36:50 | serhiy.storchaka | set | messages:
+ msg313520 |
2018-03-10 10:27:48 | steven.daprano | set | messages:
+ msg313518 |
2018-03-10 06:37:28 | tttnns | set | files:
+ simple.patch
messages:
+ msg313513 |
2018-03-09 10:39:23 | tttnns | set | messages:
+ msg313480 |
2018-03-05 12:03:50 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka, lukasz.langa messages:
+ msg313244
|
2018-03-05 11:14:02 | mcepl | set | nosy:
+ mcepl
|
2018-02-24 07:55:02 | tttnns | set | files:
- bpo-32917.patch |
2018-02-24 07:52:46 | tttnns | set | stage: patch review pull_requests:
+ pull_request5621 |
2018-02-23 09:24:39 | tttnns | set | files:
- final_blank_line.patch |
2018-02-23 09:22:13 | tttnns | set | files:
+ bpo-32917.patch
messages:
+ msg312622 |
2018-02-23 06:34:17 | tttnns | set | messages:
+ msg312619 |
2018-02-23 04:54:21 | steven.daprano | set | messages:
+ msg312616 |
2018-02-23 04:50:37 | steven.daprano | set | nosy:
+ steven.daprano messages:
+ msg312615
|
2018-02-23 01:29:50 | tttnns | create | |