Skip to content

STY: Spaces in wrong place #30781

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

Merged

Conversation

ShaharNaveh
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, couple of comments, but nothing important

"2011-01-01 10:00:00, 2011-01-01 11:00:00,\n"
" 2011-01-01 12:00:00, "
"2011-01-01 10:00:00, 2011-01-01 11:00:00,"
"\n "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but I think I'd leave the \n at the end, seems clearer and more consistent with the spaces at the end

"ああ あ\n"
" ... \n"
"ああ あ\n "
"... \n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case it's clearer the other way round. I know we can't have exceptions if we want to validate it in the CI, but I'm wondering if we could just apply the rule for a single space, and leave those other cases to common sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we can't have exceptions if we want to validate it in the CI

Actually I have thought of a way to do so (ish).

currently the script is validating if there is no space at the end of the line and a leading space at the begging of the next one, but what if it having an exception to check the number of leading spaces and according to that, the script will know if this is a style violation or an intentional.

If we say for example that if the string is starting with 3 spaces, it will consider it as intentional indentation, and will not report errors.

I am not very good with words so here is some test cases.


Will not pass:

foo = (
            "bar"
             " baz" # One space
)
foo = (
            "bar"
             "  baz" # Two spaces
)

Will pass:

foo = (
            "bar"
             "   baz" # Three spaces
)

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but doing something so complex is out of scope for pandas. I'd rather don't validate this, that making contributors have to know of such a thing.

I'd simply check the case where there are no spaces at the end of the first line, and there is just one space in the next. This will capture written paragraphs, where we should never have one space next to another, and will ignore everything else, which may include csv data and other stuff.

@WillAyd WillAyd added the Code Style Code style, linting, code_checks label Jan 7, 2020
@WillAyd WillAyd added this to the 1.0 milestone Jan 7, 2020
@ShaharNaveh ShaharNaveh force-pushed the STY-spaces-over-concat-strings-3 branch from c44f1f3 to 6e176a1 Compare January 8, 2020 17:53
" ...\n"
" NA, 1, 2, NA, 1, 2, NA, 1, 2, NA]\n"
"[ 1, 2, NA, 1, 2, NA, 1, 2, NA, 1,\n "
"...\n "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like in this case the previous option is more natural. Since every line represents the content of a line if this was the content of a file.

So, for example, if we have a file:

XX
 X

It probably makes more sense to have:

data = (
    "XX\n"
    " X\n"
)

Than

data = (
    "XX\n "
    "X\n"
)

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I do get you're point.

so if I can check if the line is ending with a \n, it's OK for the next line to begin with a space? right?

correct me if I misunderstood

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the rule could be that if one line finishes with \n, we don't really care what's in the next, it should always be ok.

But if you think it makes sense, you can revert these few lines for now, so we can merge this. And then we can see in more detail in the other PR (but I think it should just be that condition).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if you think it makes sense, you can revert these few lines for now, so we can merge this. And then we can see in more detail in the other PR (but I think it should just be that condition).

Sure!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the rule could be that if one line finishes with \n, we don't really care what's in the next, it should always be ok.

Logic for this test case is now implemented at a4ec49e

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MomIsBestFriend , let's wait for the CI, but ready to be merged.

@jreback jreback removed this from the 1.0 milestone Jan 9, 2020
@TomAugspurger TomAugspurger added this to the 1.0 milestone Jan 9, 2020
@TomAugspurger TomAugspurger merged commit 2baf788 into pandas-dev:master Jan 9, 2020
@TomAugspurger
Copy link
Contributor

Thanks!

@ShaharNaveh ShaharNaveh deleted the STY-spaces-over-concat-strings-3 branch January 10, 2020 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants