STYLE: Fix incorrect path to ujson directory
State: closed

- [x] closes #39347 

I fixed the path to `ujson` directory so `cpplint` will run on `pandas/_libs/src/ujson/` instead of `pandas/_libs/ujson`, which doesn't exist.

But the title of the issue says `cpplint (unintentionally?) skips pandas/_libs/src/ujson/`, so I understand there's a chance the `pandas/_libs/src/ujson/` is getting *intentionally* skipped.

Assuming this was **unintentional**, I believe this small fix solves the issue.

Labels:
name: IO JSON
description: read_json, to_json, json_normalize
name: Code Style
description: Code style, linting, code_checks

Comments:
Thanks @gcmaciel , but now that they are not skipped, there are lots of issues to fixup in them - is that something to want to work on?
Yeah, I'd like to work on it.
awesome! I'll mark this as "draft" then until that's done then, ping when it's ready and we'll review
@MarcoGorelli , so far I've fixed 35 out of 42 issues. But before I start working on the remaining 7, I have a few questions about them.

5 of the remaining issues says `Missing username in TODO; it should look like "// TODO(my_username): Stuff."`, which can be fixed by adding any word between those parenthesis like `TODO(username)` or `TODO(contributor)`. But I'd like to know first if this project is following some convention or anything I choose would be fine.

The other two issues has to do with a missing copyright message on two files ([date_conversions.c](https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/date_conversions.c) and [date_conversions.h](https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/date_conversions.h)). In the beginning, I had no clue which copyright message should be included, but after I inspected the other files on the same [directory](https://github.com/pandas-dev/pandas/tree/master/pandas/_libs/src/ujson/python), I've noticed all of them share the same copyright message, which is this one:
```
/*
Copyright (c) 2011-2013, ESN Social Software AB and Jonas Tarnstrom
All rights reserved.
Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
    * Redistributions of source code must retain the above copyright
      notice, this list of conditions and the following disclaimer.
    * Redistributions in binary form must reproduce the above copyright
      notice, this list of conditions and the following disclaimer in the
      documentation and/or other materials provided with the distribution.
    * Neither the name of the ESN Social Software AB nor the
      names of its contributors may be used to endorse or promote products
      derived from this software without specific prior written permission.
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL ESN SOCIAL SOFTWARE AB OR JONAS TARNSTROM BE LIABLE
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Portions of code from MODP_ASCII - Ascii transformations (upper/lower, etc)
https://github.com/client9/stringencoders
Copyright (c) 2007  Nick Galbreath -- nickg [at] modp [dot] com. All rights reserved.
Numeric decoder derived from from TCL library
https://www.opensource.apple.com/source/tcl/tcl-14/tcl/license.terms
 * Copyright (c) 1988-1993 The Regents of the University of California.
 * Copyright (c) 1994 Sun Microsystems, Inc.
*/
```
Which makes me think this is the copyright that should be included. But I'm not sure about it.
I don't know I'm afraid, this isn't something I've (yet) worked on - cc @gfyoung @WillAyd 
Thanks @WillAyd - do you know if the same copyright notice (pasted in https://github.com/pandas-dev/pandas/pull/39352#issuecomment-766404082) can be used here and needs to be included in each file?
The date_conversions files were created by pandas and not vendored from ultrajson, so they are fine without that copyright notice
Sorry approved from my phone without seeing the checks - need to be green before really approving / merging, but conceptually this is all OK
Unfortunately, without the copyright message, the linting test will fail.

The way it is right now, when I run:

 `./ci/code_checks.sh lint`

This  is the output: 

```
Linting .pyx code for spacing conventions in casting
Linting .pyx code for spacing conventions in casting DONE
Linting .c and .h
pandas/_libs/src/ujson/python/date_conversions.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
pandas/_libs/src/ujson/python/date_conversions.c:111:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
Done processing pandas/_libs/src/ujson/python/date_conversions.c
pandas/_libs/src/ujson/python/date_conversions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
pandas/_libs/src/ujson/python/date_conversions.h:17:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
Done processing pandas/_libs/src/ujson/python/date_conversions.h
pandas/_libs/src/ujson/python/objToJSON.c:1522:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
pandas/_libs/src/ujson/python/objToJSON.c:1621:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
Done processing pandas/_libs/src/ujson/python/objToJSON.c
pandas/_libs/src/ujson/python/ujson.c:76:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
Done processing pandas/_libs/src/ujson/python/ujson.c
Total errors found: 7
Linting .c and .h DONE
```

So I still need to fix that lack of copyright (and put something in that `TODO` part).
If needed for cpplint you can use the copyright here

https://github.com/pandas-dev/pandas/blob/309cf3a6c76c0671ac5710e14655257c9cbcdbf8/pandas/_libs/src/inline_helper.h#L2

Just change 2016 to whenever the file was created through present

Sent from my iPad

> On Jan 24, 2021, at 11:19 AM, Gustavo C. Maciel <notifications@github.com> wrote:
> 
> ﻿
> Unfortunately, without the copyright message, the linting test will fail.
> 
> The way it is right now, when I run:
> 
> ./ci/code_checks.sh lint
> 
> This is the output:
> 
> Linting .pyx code for spacing conventions in casting
> Linting .pyx code for spacing conventions in casting DONE
> Linting .c and .h
> pandas/_libs/src/ujson/python/date_conversions.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
> pandas/_libs/src/ujson/python/date_conversions.c:111:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
> Done processing pandas/_libs/src/ujson/python/date_conversions.c
> pandas/_libs/src/ujson/python/date_conversions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
> pandas/_libs/src/ujson/python/date_conversions.h:17:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
> Done processing pandas/_libs/src/ujson/python/date_conversions.h
> pandas/_libs/src/ujson/python/objToJSON.c:1522:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
> pandas/_libs/src/ujson/python/objToJSON.c:1621:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
> Done processing pandas/_libs/src/ujson/python/objToJSON.c
> pandas/_libs/src/ujson/python/ujson.c:76:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
> Done processing pandas/_libs/src/ujson/python/ujson.c
> Total errors found: 7
> Linting .c and .h DONE
> So I still need to fix that lack of copyright (and put something in that TODO part).
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Great! Thanks @WillAyd 
- [x] closes #39347 
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them

I fixed all the issues raised by `cpplint`. 

But one of the changes I've made led to some fails. After inspecting those logs I've identified that when I've replaced `sprintf` with `snprinf` at [objToJSON.c](https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/objToJSON.c), line 1399, to fix the issue `Never use sprintf. Use snprintf instead.  [runtime/printf] [5]`, it led to these errors when it tried to build pandas `error: passing argument 2 of ‘snprintf’ makes integer from pointer without a cast [-Werror=int-conversion]` and `error: passing argument 3 of ‘snprintf’ makes pointer from integer without a cast [-Werror=int-conversion]`. 

So to make it work, I reverted `snprinf` to `sprintf` so that function will keep working as it was before, and, in `code_checks.sh`, I added this new filter `-runtime/printf` to the list of filters so `cpplint` will not raise the `Never use sprintf.  Use snprintf instead` error again. I also included this new line of comment:

```
    # runtime/printf: Warnings about using sprintf instead of snprintf
```
It's all green now! @MarcoGorelli @WillAyd , this PR is ready to be reviewed 
The error messages suggest you are just mixing up passing a pointer and an integer as an argument. Can you post the code tried instead of removing the rule?

Sent from my iPhone

> On Jan 25, 2021, at 7:47 AM, Gustavo C. Maciel <notifications@github.com> wrote:
> 
> ﻿
> It's all green now! @MarcoGorelli @WillAyd , this PR is ready to be reviewed
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

@WillAyd sure, but actually I wasn't writing a new function or anything, I only replaced `sprintf` with `snprintf` because it was required by `cpplint`. So this is the code from https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/objToJSON.c#L1398, line 1398, before the replacement:
```c
cLabel = PyObject_Malloc(21); // 21 chars for int64
sprintf(cLabel, "%" NPY_DATETIME_FMT,
        NpyDateTimeToEpoch(nanosecVal, base));
len = strlen(cLabel);
```
Then, I've replaced `sprintf` with `snprintf` and it looked like this:
```c
cLabel = PyObject_Malloc(21); // 21 chars for int64
snprintf(cLabel, "%" NPY_DATETIME_FMT,
        NpyDateTimeToEpoch(nanosecVal, base));
len = strlen(cLabel);
```
But this second code led to those errors. I tried to find a way to make it work without removing the rule but, in the end, I decided to  revert that to `sprinft` (which was working before) and add `-runtime/printf` to the list of filters because I've noticed there were some other rules that had been already removed and also because at least it could have a working version until this could be better discussed.
The second argument to snprintf is an integer. I believe you can just pass 21 as the second argument (the size of the cLabel buffer)

> On Jan 25, 2021, at 9:59 AM, Gustavo C. Maciel <notifications@github.com> wrote:
> 
> 
> @WillAyd <https://github.com/WillAyd> sure, but actually I wasn't writing a new function or anything, I only replaced sprintf with snprintf because it was required by cpplint. So this is the code from https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/objToJSON.c#L1398 <https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/ujson/python/objToJSON.c#L1398>, line 1398, before the replacement:
> 
> cLabel = PyObject_Malloc(21); // 21 chars for int64
> sprintf(cLabel, "%" NPY_DATETIME_FMT,
> NpyDateTimeToEpoch(nanosecVal, base));
> len = strlen(cLabel);
> Then, I've replaced sprintf with snprintf and it looked like this:
> 
> cLabel = PyObject_Malloc(21); // 21 chars for int64
> snprintf(cLabel, "%" NPY_DATETIME_FMT,
> NpyDateTimeToEpoch(nanosecVal, base));
> len = strlen(cLabel);
> But this second code led to those errors. I tried to find a way to make it work without removing the rule but, in the end, I decided to revert that to sprinft (which was working before) and add -runtime/printf to the list of filters because I've noticed there were some other rules that had been already removed and also because at least it could have a working version until this could be better discussed.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/39352#issuecomment-767002588>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU4UIYBKAA6QXMNR33OLTS3WWQLANCNFSM4WP4XR2A>.
> 


Like this, right?

```c
cLabel = PyObject_Malloc(21); // 21 chars for int64
snprintf(cLabel, 21, "%" NPY_DATETIME_FMT,
        NpyDateTimeToEpoch(nanosecVal, base));
len = strlen(cLabel);
```
I think looks right - can you compile and test?

Sent from my iPhone

> On Jan 25, 2021, at 11:53 AM, Gustavo C. Maciel <notifications@github.com> wrote:
> 
> ﻿
> Like this, right?
> 
> cLabel = PyObject_Malloc(21); // 21 chars for int64
> snprintf(cLabel, 21, "%" NPY_DATETIME_FMT,
>         NpyDateTimeToEpoch(nanosecVal, base));
> len = strlen(cLabel);
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

It didn't work as expected because `cpplint` raised this error `If you can, use sizeof(cLabel) instead of 21 as the 2nd arg to snprintf.  [runtime/printf] [3]`, so I went there and replaced `21` with `sizeof(cLabel)`, but if failed the test when it tried to build pandas giving this error `error: ‘clabel’ undeclared (first use in this function)`.

So I decided to declare `int size_of_cLabel = 21` and pass it to `snprintf`, which is basically the same as passing `21` to the function, but this time `cpplint` won't raise any error. And, as expected, it worked.

The final version looks like this:

```c
cLabel = PyObject_Malloc(21); // 21 chars for int64
int size_of_cLabel = 21;
snprintf(cLabel, size_of_cLabel, "%" NPY_DATETIME_FMT,
        NpyDateTimeToEpoch(nanosecVal, base));
len = strlen(cLabel);
```
Now I'm going to remove `-runtime/printf` from the list of filters (which I also agree it wasn't the optimal solution) and push the changes here (I'll keep this as a draft until then).


Can you push the change? Would be a little cleaner if you move the variable declaration up one line and pass it as an argument to `PyObject_Malloc` as well
I pushed the changes. 

I'm pasting here too to make it easier:

```c
int size_of_cLabel = 21;  // 21 chars for int 64
cLabel = PyObject_Malloc(size_of_cLabel);
snprintf(cLabel, size_of_cLabel, "%" NPY_DATETIME_FMT,
        NpyDateTimeToEpoch(nanosecVal, base));
len = strlen(cLabel);
Great thanks @gcmaciel