Skip to content

Conversation

fiedukow
Copy link
Contributor

In test_36_unichars we are writing local python file and them import it for testing purposes.

So far this was done in a local directory (current working dir), but this is not ideal, as it might not be on a writable filesystem. For that reason it seems better to use a temporary directory, that is much more commonly provided in a writable form.

This PR proposes that shift.

@apalala
Copy link
Collaborator

apalala commented Jun 24, 2025

This seems to apply only to parameter_test.py.

I have no objections to the improvements except that if they are useful the module should use pathlib instead of os.path.

@fiedukow fiedukow force-pushed the afie/tempDirFileInTest branch from fe20da6 to d589ad9 Compare June 24, 2025 14:34
@fiedukow
Copy link
Contributor Author

Done, converted this from os to pathlib.

@apalala
Copy link
Collaborator

apalala commented Jun 24, 2025

One more thing, @fiedukow...

Please add yourself to the list of contributors under .\docs.

@fiedukow fiedukow force-pushed the afie/tempDirFileInTest branch from d589ad9 to 34e2a10 Compare June 24, 2025 19:36
@fiedukow
Copy link
Contributor Author

Thanks, done!

@apalala
Copy link
Collaborator

apalala commented Jun 24, 2025

The reference as contributor is not complete, @fiedukow. There's another file you have to modify so the your name resolves to a link.

In `test_36_unichars` we are writing local python file and them import it for
testing purposes.

So far this was done in a local directory (current working dir), but this is
not ideal, as it might not be on a writable filesystem. For that reason it
seems better to use a temporary directory, that is much more commonly provided
in a writable form.

This change makes that shift.
@fiedukow fiedukow force-pushed the afie/tempDirFileInTest branch from 34e2a10 to 2bae900 Compare June 24, 2025 20:19
@fiedukow
Copy link
Contributor Author

Sorry for that, I missed that file. Hopefully fixed now 🙏

@apalala apalala merged commit 404dbe8 into neogeny:master Jun 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants