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

Adding validator for files on converter to cloud project #614

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

SeqLaz
Copy link
Member

@SeqLaz SeqLaz commented Sep 21, 2024

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

I would add such function in file utils.

Also the pattern is a bit more complex, expecially for windows. Can we use something like this:

\A(?!(?:COM[0-9]|CON|LPT[0-9]|NUL|PRN|AUX|com[0-9]|con|lpt[0-9]|nul|prn|aux)|\s|[\.]{2,})[^\\\/:*"?<>|]{1,254}(?<![\s\.])\z

Even better, we can check if pathvalidate is installed and use that, otherwise fallback to the above regex (or better one). Don't forget to check if the file path is also correct.

@SeqLaz
Copy link
Member Author

SeqLaz commented Sep 23, 2024

I would add such function in file utils.

Also the pattern is a bit more complex, expecially for windows. Can we use something like this:

\A(?!(?:COM[0-9]|CON|LPT[0-9]|NUL|PRN|AUX|com[0-9]|con|lpt[0-9]|nul|prn|aux)|\s|[\.]{2,})[^\\\/:*"?<>|]{1,254}(?<![\s\.])\z

Even better, we can check if pathvalidate is installed and use that, otherwise fallback to the above regex (or better one). Don't forget to check if the file path is also correct.

Thank you. Due to the library pathvalidate not being installed, I used your regex for the validations and moved the function to /utils/file_utils.py.

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Can we use the ProjectChecker for providing this kind of feedback? There is already ASCII filename warnings, the file one to be added should be an error that prevents further actions with the project.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 4, 2024

ProjectChecker

Can we use the ProjectChecker for providing this kind of feedback? There is already ASCII filename warnings, the file one to be added should be an error that prevents further actions with the project.

For this it will be require to add a method to libqfieldsync

@suricactus
Copy link
Collaborator

Can you add a screencast how this works? Code looks good enough for now.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 19, 2024

Can you add a screencast how this works? Code looks good enough for now.

@suricactus There in the up is with when the message is directly on QFieldSync on the down when is using the libqfieldsync

Using directly QFieldSync

directly_on_qfield_sync.mp4

Using libqfieldsync

using_libqfieldsync.mp4

My vote is for opt for the libqfieldsync option, also is already for create a PR

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

I like the second option too! Thanks, good work! Please link the libqfieldsync PR here when ready.

@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 22, 2024

I like the second option too! Thanks, good work! Please link the libqfieldsync PR here when ready.

If we opt for the second option we can close this PR, all the logic is managed in this PR opengisch/libqfieldsync#95

@SeqLaz SeqLaz closed this Oct 22, 2024
@suricactus
Copy link
Collaborator

Actually you can reopen this and just bump your linqfieldsync commit version.

@suricactus suricactus reopened this Oct 23, 2024
@SeqLaz SeqLaz force-pushed the adding_check_for_forbidden_characters branch from 3ab98ca to 05c384a Compare October 24, 2024 17:19
@SeqLaz
Copy link
Member Author

SeqLaz commented Oct 24, 2024

Hey @suricactus I removed all the previous commits and add the last master libqfieldsync as part of the requirements, on the last master was merged the PR

@suricactus suricactus merged commit 3642b16 into master Nov 12, 2024
7 checks passed
@suricactus suricactus deleted the adding_check_for_forbidden_characters branch November 12, 2024 19:57
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