-
Notifications
You must be signed in to change notification settings - Fork 508
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
Skip over lines that don't set an envvars #291
Conversation
This makes dotenv play better with .env files that do more than just set variables, e.g. defining aliases and functions closes #285
@@ -59,8 +59,6 @@ def parse_line(line) | |||
if variable_not_set?(line) | |||
raise FormatError, "Line #{line.inspect} has an unset variable" | |||
end | |||
elsif line !~ /\A\s*(?:#.*)?\z/ # not comment or blank line | |||
raise FormatError, "Line #{line.inspect} doesn't match format" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to warn
on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong feeling about it, but I would think not. We don't expect every line to follow this format, so we shouldn't raise a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just warn once when the file parsing is finished 😁?
@bkeepers Are there any further changes that need to be made before this is merged? Would you like me to add warning(s), or is this ok as is? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I still think this implements the correct behavior. Please let me know if there are any further changes to be made. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hi @sloria, I'm taking over maintenance of dotenv with @cadwallion and @JessRudder and your PR is on our radar. Were you still interested in seeing this shipped? If so I will re-open, review, and respond or merge. Thank you for your time 😄 |
@jonmagic Thanks for the response. I opened this issue because I ran into it when using docker-sync. See EugenMayer/docker-sync#254 . I was able to work around the issue by changing the file that autoenv reads, but that's a bit of a hack. I still think this PR implements the correct behavior, so I think you should consider reviewing/merging it. |
@sloria excellent, thank you for the response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
We'll be cutting a new release next week that will include the changes from this PR. |
This makes dotenv play better with .env files that
do more than just set variables, e.g. defining aliases
and functions
closes #285