Skip to content
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

Allow to use ijson package by a relative import #93

Open
prrvchr opened this issue Apr 15, 2023 · 4 comments
Open

Allow to use ijson package by a relative import #93

prrvchr opened this issue Apr 15, 2023 · 4 comments
Labels

Comments

@prrvchr
Copy link

prrvchr commented Apr 15, 2023

Hi,

I am using ijson package to stream json data coming from Request API.

I do this in a LibreOffice/OpenOffice extension (a file with the oxt extension) in which is a copy of the ijson package.

In order to remain independent of the system on which this extension is installed, I try if possible to use relative imports on my necessary python packages.
This is only possible if the python package you are trying to import also only uses relative imports.

This mode of use brings several improvements:

  • This makes it possible to be sure to always have the same version regardless of the versions already installed on the host system.
  • This allows you to embed the package in a python program without necessarily having to install the package at the system python level.
  • This allows the use of the package without it being installed in the PYTHONPATH.
  • It allows version management.

I only see advantages in it, and it should normally also work with Python version 2.7, and in fact the changes are minimal, they are visible on this link.

Anyway thanks again for your API.

@rtobar
Copy link

rtobar commented Apr 17, 2023

I'll answer quickly now, as I'm about to go on leave for a couple of weeks: not entirely closed to the idea, but given the additional maintenance burden it would require more work to ensure: a) all imports have been taken into account (C extension?) and b) we don't accidentally add an absolute import in the future

@prrvchr
Copy link
Author

prrvchr commented Apr 18, 2023

Hi rtobar,

I'm happy to know that you're going on vacation and I wish you all the best...

I have already taken care of the changes for the python code, on the other hand, for the C code, nothing has been done and I am not able to do it... And I don't know how to test all this part of the code in C...

We'll talk about it after your return, see you soon.

@rtobar
Copy link

rtobar commented May 9, 2023

@prrvchr thanks for the wait.

I have been thinking more about this, and I think I'll pass. The patch you produced looks good, and I experimented with the necessary changes in the C backend too, which wasn't too difficult either, so making these initial changes isn't an issue. For reference, this seems to work for the C backend:

diff --git a/ijson/backends/yajl2_c/module.c b/ijson/backends/yajl2_c/module.c
index 581047c..85d202f 100644
--- a/ijson/backends/yajl2_c/module.c
+++ b/ijson/backends/yajl2_c/module.c
@@ -107,7 +107,8 @@ MOD_INIT(_yajl2)
        INIT_ENAME(end_array);
 
        // Import globally-used names
-       PyObject *ijson_common = PyImport_ImportModule("ijson.common");
+       PyObject *ijson_common = PyImport_ImportModuleLevel("common", PyModule_GetDict(m), NULL, NULL, 2);
+       X_N(ijson_common, MOD_VAL(NULL));
        PyObject *decimal_module = PyImport_ImportModule("decimal");
        X_N(ijson_common, MOD_VAL(NULL));
        X_N(decimal_module, MOD_VAL(NULL));

What I do worry about is maintainability: unless we have a good way to automatically check that this support isn't broken, we'll eventually break it, invalidating any changes we do now. And I am not really willing to put that much effort into developing such tests. They would ideally involve copying all the ijson/ subdirectory into a new my_project top-level package, then install that new package, then run the ijson tests importing ijson from that package. This is not super, heavily complicated, but isn't trivial either, specially considering that we'd need to compile the C extension to make sure that works as expected too.

So all in all I don't think it's a bad idea, but I'm not ready to spend the effort that would be required to implement it properly. If you'd like to give it a go then I could review any proposed changes, but otherwise let me know and I'll close this ticket as not planned.

@prrvchr
Copy link
Author

prrvchr commented May 9, 2023

I would be happy to participate and try to finish the integration, and I understand the need to integrate these changes into the tests.
On the other hand, I suppose that this new test must be integrated into already existing tests?
And I understand the fact of having to copy to a subfolder to perform the tests, but what do you mean to install that new package?
Or could we not consider modifying the existing tests in order to perform them on a subfolder by relative imports?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants