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

Put conf_file loop outside of init_file loop #666

Merged
merged 1 commit into from
Jan 10, 2019

Conversation

thalesmello
Copy link
Contributor

Addreses #665

Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks, @thalesmello

@derekstavis
Copy link
Member

derekstavis commented Jan 4, 2019

Thanks for taking the time to debug and fix this bug!

The loop was done on purpose. As you can see, globs are used thoroughly in require function, and it is used mainly because it is a performant way of checking for the existence of one or more files without forking to a subprocess or calling test (which is prohibitive in the init code path).

The $init_path variable in this case is populated with all init.fish files present in package directory, which should return one and only one file (yes, prepending anything after .fish will give bogus stuff). Further in the code, if the given init.fish file was already sourced, we skip loading again, what would avoid reloading conf.d twice.

I noticed that this PR fixes another bug that I didn't notice when adding conf.d loading! Currently conf.d is only being loaded when init.fish is present, which is not what we wanted to achieve.

I'm not gonna block the merge, but I will leave some concerns that we might want to address in the future:

  1. The mechanism that will not source init.fish (or init files, which includes conf.d) is not effective for conf.d anymore (https://github.com/oh-my-fish/oh-my-fish/blob/master/lib/require.fish#L55)
  2. The benchmarking events will not take conf.d into account anymore (https://github.com/oh-my-fish/oh-my-fish/blob/master/lib/require.fish#L84)

@derekstavis derekstavis merged commit c0e69a5 into oh-my-fish:master Jan 10, 2019
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.

None yet

3 participants