Remove new line characters after last row of data in ascii.latex.AASTex #4561
Conversation
|
@taldcroft : This looks good to me (Tracis failure is unrelated). @bsipocz : There is a failure of the coverage test here, which I'm very sure is unrelated to the content of the PR. Do you now if that is a ci-helpers issue? @anchitjain1234 : Just a small tip: If you include the string |
|
We fixed the coverage earlier today, will restart that build. |
|
Thanks @hamogu for reviewing. In which version should the changelog entry be added? |
|
This is an easy bug fix with no risk of merge conflicts. It should go in 1.0.9. |
anchitjain1234
force-pushed
the
fix-3888
branch
from
bda5d51 to
d142624
Compare
anchitjain1234
force-pushed
the
fix-3888
branch
from
d142624 to
848f2ba
Compare
| core.BaseData.write(self, lines) | ||
| # To remove extra space and // appended which creates an extra new line | ||
| # in the end. | ||
| if len(lines) > lines_length_initial: |
There was a problem hiding this comment.
There is probably a reason why not, but could you just do (without the len(lines) > lines_length_initial test):
lines[-1] = re.sub(r'//\s*$', '', lines[-1])
I guess I'm not sure about a corner case of a zero-length table or something.
There was a problem hiding this comment.
I thought about that, too, when I reviewed it, and decided that the ways it's implemented now is better for exactly the case of an empty table you mention. I don't think that ever comes up, but we should not put in bug just for the sake of saving one line of code (and this is not performance critical - nobody writes millions of AASTeX tables).
There was a problem hiding this comment.
So it should be left as it is?
There was a problem hiding this comment.
What about a hybrid of using the regex within the existing if statement. I always get nervous about things like [:-3] since that is just begging to break if anything changes upstream. I know it's not likely in this case, but in theory someone could make a sub-class and change join so that it adds just r'\\' (without the space).
Also, I guess you would want the regex to be r'\s* \\ \s* $' (with re.VERBOSE).
|
@taldcroft @hamogu Please check changes now. |
|
I thought the old version was good enough. This is even better. |
|
@taldcroft Please check the changes. |
Remove new line characters after last row of data in ascii.latex.AASTex
|
Thanks @anchitjain1234 ! |
|
@anchitjain1234 : Sometimes there is some patience required. Almost all of us work on astropy as a hobby in our free time and @taldcroft is responsible for > 100 of the PRs and issues. |
|
@hamogu @taldcroft Sorry for spamming 😞. Won't happen again. |
|
@anchitjain1234 : Don't worry. That's OK. And sometimes we do forget about open PRs and then it's a good idea to ask again; I was just explaining why we might not respond immediately. |
Remove new line characters after last row of data in ascii.latex.AASTex
Remove new line characters after last row of data in ascii.latex.AASTex
|
This change broke reading of AASTex deluxetable files. The line parser requires the final terminating '' on each line, including the last one. E.g., the following has been in since The parser is wrong, but that's a separate issue. We should at least preserve round-tripping and add that to the test suite. Bug reported in #5160 |
5 participants
For #3888. Also fixed
AASTextests intest_write.pyto incorporate these changes.