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

For toppings avoid issues with backslashes #120

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Conversation

signedav
Copy link
Member

On windows backslashes where created with topping exporter file paths.
This leaded to issues when the relative path has been attached to an url.

The IliDataCache.download_file now:

  • replaces the backslashes with slashes on URL
  • normalizes the path to the current system

Still ili2db does not that.

The IliToppingMaker creating those paths is now storing everything with backslashes as well, nerveless if it's an url or a file path (because it cannot know that).

Windows can handel slashes in the path as well AFAIK, what means it's more stable to have slashes.

This leaded to issues when the relative path has been attached to an url.

The IliDataCache.download_file now:
- replaces the backslashes with slashes on URL
- normalizes the path to the current system

Still ili2db does not that.

The IliToppingMaker creating those paths is now storing everything with backslashes as well, nerveless if it's an url or a file path (because it cannot know that).

Windows can handel slashes in the path as well AFAIK, what means it's more stable to have slashes.
Copy link
Collaborator

@gacarrillor gacarrillor left a comment

Choose a reason for hiding this comment

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

Do you plan to adjust the topping exporter URL/path generation as well? For instance, replacing the os module by pathlib?

@signedav
Copy link
Member Author

In the "modelbaker" part ilitoppingmaker.Target I just replaced the backslashes. Are there better ways to do it with pathlib maybe I've overseen that, but I could not find kind of an urinormalizer.

The library toppingmaker I leave like it is. Because people might use it primarily locally.

@gacarrillor
Copy link
Collaborator

In the "modelbaker" part ilitoppingmaker.Target I just replaced the backslashes. Are there better ways to do it with pathlib maybe I've overseen that, but I could not find kind of an urinormalizer.

As far as I know, you'd need to tell pathlib you've got a pure Windows path and then it can normalize it. Something like:

p = pathlib.PureWindowsPath("my\\path")
p.as_posix()

or

p = pathlib.PureWindowsPath(r"my\path")
p.as_posix()

@signedav
Copy link
Member Author

But the question is in the end how you write it to the yaml (or the ilidata file) and there you have to decide if it's slash or backslash or am I not getting something?

@gacarrillor
Copy link
Collaborator

But the question is in the end how you write it to the yaml (or the ilidata file) and there you have to decide if it's slash or backslash or am I not getting something?

I think I'm missing something since the beginning :). That's why I wrote the question.
If you can standardize the way paths get written into the yaml or ilidata files, that would be the way to go in my opinion. Probably I'm missing some scenarios, because I thought that choosing forward slashes there would always be preferred (or even enforced).

@signedav
Copy link
Member Author

ah no, you are right. I got confused by the PureWindowsPath and I thought it creates a windows path out of it (what is the opposite of what I wanted). But it does exactly what we need. Thanks.

@signedav signedav merged commit 3929f37 into main Nov 15, 2024
5 checks passed
@signedav signedav deleted the slash-vs-backslash branch November 15, 2024 12:48
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