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

Remove new line characters after last row of data in ascii.latex.AASTex #4561

Merged
merged 3 commits into from Feb 8, 2016

Conversation

Projects
None yet
5 participants
@anchitjain1234
Contributor

anchitjain1234 commented Feb 3, 2016

For #3888. Also fixed AASTex tests in test_write.py to incorporate these changes.

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 4, 2016

Member

@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 closes #xxxx or fixes #xxxx in a commit message then you automatically reference that issue on github and that issue will be closed automatically when your PR is merged.

Member

hamogu commented Feb 4, 2016

@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 closes #xxxx or fixes #xxxx in a commit message then you automatically reference that issue on github and that issue will be closed automatically when your PR is merged.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog Feb 4, 2016

Member

We fixed the coverage earlier today, will restart that build.

Member

astrofrog commented Feb 4, 2016

We fixed the coverage earlier today, will restart that build.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 5, 2016

Contributor

Thanks @hamogu for reviewing. In which version should the changelog entry be added?

Contributor

anchitjain1234 commented Feb 5, 2016

Thanks @hamogu for reviewing. In which version should the changelog entry be added?

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 5, 2016

Member

This is an easy bug fix with no risk of merge conflicts. It should go in 1.0.9.

Member

hamogu commented Feb 5, 2016

This is an easy bug fix with no risk of merge conflicts. It should go in 1.0.9.

Show outdated Hide outdated astropy/io/ascii/latex.py
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:

This comment has been minimized.

@taldcroft

taldcroft Feb 5, 2016

Member

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.

@taldcroft

taldcroft Feb 5, 2016

Member

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.

This comment has been minimized.

@hamogu

hamogu Feb 5, 2016

Member

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).

@hamogu

hamogu Feb 5, 2016

Member

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).

This comment has been minimized.

@anchitjain1234

anchitjain1234 Feb 5, 2016

Contributor

So it should be left as it is?

@anchitjain1234

anchitjain1234 Feb 5, 2016

Contributor

So it should be left as it is?

This comment has been minimized.

@taldcroft

taldcroft Feb 5, 2016

Member

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

taldcroft Feb 5, 2016

Member

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).

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 5, 2016

Contributor

@taldcroft @hamogu Please check changes now.

Contributor

anchitjain1234 commented Feb 5, 2016

@taldcroft @hamogu Please check changes now.

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 5, 2016

Member

I thought the old version was good enough. This is even better.

Member

hamogu commented Feb 5, 2016

I thought the old version was good enough. This is even better.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 8, 2016

Contributor

@taldcroft Please check the changes.

Contributor

anchitjain1234 commented Feb 8, 2016

@taldcroft Please check the changes.

@taldcroft taldcroft added this to the v1.0.9 milestone Feb 8, 2016

@taldcroft taldcroft self-assigned this Feb 8, 2016

taldcroft added a commit that referenced this pull request Feb 8, 2016

Merge pull request #4561 from anchitjain1234/fix-3888
Remove new line characters after last row of data in ascii.latex.AASTex

@taldcroft taldcroft merged commit b4f30c9 into astropy:master Feb 8, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 76.761%
Details
@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft
Member

taldcroft commented Feb 8, 2016

Thanks @anchitjain1234 !

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 8, 2016

Member

@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.

Member

hamogu commented Feb 8, 2016

@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.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 8, 2016

Contributor

@hamogu @taldcroft Sorry for spamming 😞. Won't happen again.

Contributor

anchitjain1234 commented Feb 8, 2016

@hamogu @taldcroft Sorry for spamming 😞. Won't happen again.

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 8, 2016

Member

@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.

Member

hamogu commented Feb 8, 2016

@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.

eteq added a commit that referenced this pull request Feb 24, 2016

Merge pull request #4561 from anchitjain1234/fix-3888
Remove new line characters after last row of data in ascii.latex.AASTex

eteq added a commit that referenced this pull request Feb 29, 2016

Merge pull request #4561 from anchitjain1234/fix-3888
Remove new line characters after last row of data in ascii.latex.AASTex

@anchitjain1234 anchitjain1234 deleted the anchitjain1234:fix-3888 branch Apr 8, 2016

@wmwv

This comment has been minimized.

Show comment
Hide comment
@wmwv

wmwv Jul 6, 2016

Contributor

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 ee9c71bf (2014-08-02):

https://github.com/astropy/astropy/blob/master/astropy/io/ascii/latex.py#L77

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

Contributor

wmwv commented Jul 6, 2016

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 ee9c71bf (2014-08-02):

https://github.com/astropy/astropy/blob/master/astropy/io/ascii/latex.py#L77

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

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