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

Two commits (can be cherrypicked): Custom Env vars for Perl in settings, and convert tilda to home in paths #119

Merged
merged 2 commits into from
Apr 6, 2024

Conversation

nugged
Copy link
Contributor

@nugged nugged commented Mar 21, 2024

Feature 1:

Add support for custom Perl environment variables

Introduced two new environment variables: perlEnv and perlEnvAdd.

  • perlEnv: This is a hash to fully override the system's environment for PerlNavigator.
  • perlEnvAdd: This is a hash to add or modify specific environment variables for PerlNavigator.

Feature 2:

Add support for expanding leading tildas in paths

  • if needed: variable to On/Off that.

@nugged
Copy link
Contributor Author

nugged commented Mar 21, 2024

NOTE: I tested this under MacOS (no Windows and I suspect it might be a different way of substituting Windows "home" or other relativeness).

@bscan
Copy link
Owner

bscan commented Mar 21, 2024

This looks great, thanks for putting this together! I'll do some testing this weekend and merge it in. Some comments:

I don't think we'd need to disable the tilde expansion. Currently, tilde's simply don't work so having it on all the time is probably the best. Looks like they considered adding it in node, but decided against it: nodejs/node#684
And people seem to recommend untildify, which has 14 million weekly downloads and does the same thing ( os.homedir ) https://github.com/sindresorhus/untildify/blob/a901e25ef782d93df1eba04ed48f56c79d157c74/index.js#L10
Although I greatly prefer your approach over bringing in a dependency for one line of code.

On Windows, the equivalent is probably %USERPROFILE% although far less used. We can probably just ignore it for now. I can test on Windows and Linux just to make sure nothing breaks, but I think it would be fine.

PerlEnv and PerlEnvAdd both make sense as well. I suspect Add is the more common case, where someone just needs to add or override a variable. Clearing out variables might have some unexpected effects, especially if someone's perl setup uses PERL5LIB or a PERLCRITIC profile. We can use the README to point people toward preferring PerlEnvAdd if needed.

Thanks!

Introduced two new environment variables: perlEnv and perlEnvAdd.

- perlEnv: Hash. Pass environment variables to the perl executable. Skipped if undefined.
- perlEnvAdd: Boolean. Add environment variables to current environment, or totally replace (perlEnv related). Default is true.
@nugged nugged force-pushed the env_vars_and_tilda_in_paths branch from 67b2d7f to 3bf5680 Compare March 22, 2024 03:08
@nugged
Copy link
Contributor Author

nugged commented Mar 22, 2024

PerlEnv and PerlEnvAdd both make sense as well. I suspect Add is the more common case, where someone just needs to add or override a variable. Clearing out variables might have some unexpected effects, especially if someone's perl setup uses PERL5LIB or a PERLCRITIC profile. We can use the README to point people toward preferring PerlEnvAdd if needed.

Good point. I reworked the idea of PerlEnv: two variables:

  • perlEnv: Hash. Pass environment variables to the perl executable. Skipped if undefined.
  • perlEnvAdd: Boolean. Add environment variables to the current environment, or totally replace (perlEnv related). The default is true.

More clean, and added by default, and perlEnvAdd can be used rarely.

@nugged nugged force-pushed the env_vars_and_tilda_in_paths branch from 3bf5680 to 3e9af99 Compare March 22, 2024 03:12
@nugged
Copy link
Contributor Author

nugged commented Mar 22, 2024

Also: removed

Add expandLeadTildasInPaths Setting for Optional Tilda Expansion

commit at all, no need to on/off tildas' expansion.

@nugged nugged force-pushed the env_vars_and_tilda_in_paths branch from 3e9af99 to 53e29c5 Compare March 22, 2024 03:16
@nugged
Copy link
Contributor Author

nugged commented Mar 22, 2024

... and removed Windows "placeholder" ~\\ which is actually not.

This commit introduces functionality to expand leading tildas (~) in paths. The tilde is a common shorthand for the home directory in Unix-like operating systems, and this change allows our application to interpret and handle these paths correctly.

Previously, if a user entered a path with a leading tilde, the application would not recognize it as a valid path. With this update, the application will now expand any leading tilde to the current user's home directory, allowing for more flexible and intuitive path input.

This change improves the user experience by accepting a wider range of valid inputs and aligns the application's path handling behavior with established conventions in Unix-like operating systems.
@nugged nugged force-pushed the env_vars_and_tilda_in_paths branch from 53e29c5 to d439d93 Compare March 22, 2024 03:51
@bscan bscan merged commit ddc803b into bscan:main Apr 6, 2024
@bscan
Copy link
Owner

bscan commented Apr 6, 2024

This is great, my apologies for the slow merge. Thanks again for putting this together!

@nugged nugged deleted the env_vars_and_tilda_in_paths branch April 6, 2024 10:09
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