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

[Feat] default to XDG_PICTURES_DIR for save path #124

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

Uzaaft
Copy link
Contributor

@Uzaaft Uzaaft commented Jul 24, 2024

@mistricky I think this is a better, more saner default were we default to the xdg_pictures_dir or /Users/<Username>/Pictures on mac. Let me know what you think. I tried to update the docs where applicable, not sure if more is required.

@Uzaaft Uzaaft force-pushed the main branch 6 times, most recently from 0b2b3b6 to 4b2755e Compare July 24, 2024 11:59
@Uzaaft
Copy link
Contributor Author

Uzaaft commented Jul 24, 2024

I'm not sure why, but I get this random as TOOD whenever I run it now:
image

@Uzaaft Uzaaft marked this pull request as ready for review July 24, 2024 17:02
@Uzaaft
Copy link
Contributor Author

Uzaaft commented Jul 24, 2024

Fixed the issue above by just moving to pure lua space.

@mistricky
Copy link
Owner

@mistricky I think this is a better, more saner default were we default to the xdg_pictures_dir or /Users/<Username>/Pictures on mac. Let me know what you think. I tried to update the docs where applicable, not sure if more is required.

Hi @Uzaaft, I think set default value of save_path is great, but maybe there should have more logic for handle edge case, for instance, if the default path does not exist

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Jul 25, 2024

@mistricky I think this is a better, more saner default were we default to the xdg_pictures_dir or /Users/<Username>/Pictures on mac. Let me know what you think. I tried to update the docs where applicable, not sure if more is required.

Hi @Uzaaft, I think set default value of save_path is great, but maybe there should have more logic for handle edge case, for instance, if the default path does not exist

What do you mean with edge cases? Isn't the edge cases done here, i.e defaulting to xdg, enough?

@mistricky
Copy link
Owner

@mistricky I think this is a better, more saner default were we default to the xdg_pictures_dir or /Users/<Username>/Pictures on mac. Let me know what you think. I tried to update the docs where applicable, not sure if more is required.

Hi @Uzaaft, I think set default value of save_path is great, but maybe there should have more logic for handle edge case, for instance, if the default path does not exist

What do you mean with edge cases? Isn't the edge cases done here, i.e defaulting to xdg, enough?

I mean if somehow the value of XDG_PICTURES_DIR does not exists on disk, or if someone removed the ~/Pictures on his Mac, we should tell users what happened, and what they can do in this case.

Without default save_path, users should specify save_path explicitly, so the above case shouldn't happen, but if we set default value, we should check and throw more useful error message to users

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Jul 25, 2024

I believe the error that will appear if the folder doesn't exist will be as useful as the one you provide today. I.e:

Value of type () couldn't be pushed on the stack: Lua runtime error: No such file or directory (os error 2).

Copy link
Owner

@mistricky mistricky left a comment

Choose a reason for hiding this comment

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

LGTM

@mistricky mistricky merged commit f49e3bd into mistricky:main Jul 30, 2024
@mistricky mistricky changed the title feat: Default to XDG_PICTURES_DIR for save path [Feat] default to XDG_PICTURES_DIR for save path Jul 30, 2024
Uzaaft added a commit to AstroNvim/astrocommunity that referenced this pull request Jul 30, 2024
ALameLlama pushed a commit to AstroNvim/astrocommunity that referenced this pull request Jul 30, 2024
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