Expose readConfigs #121

2018-02-23T12:05:11+01:00

AlexeyRaga commented

2018-02-23 12:05:11 +01:00

(Migrated from github.com)

Changes

Exposed functions readConfigs and defaultConfigPaths so that external tools can make use of them.
Updated Main.hs to call exposed readConfigs with defaultConfigPaths (command line provided paths still take priority).

## Changes - Exposed functions `readConfigs` and `defaultConfigPaths` so that external tools can make use of them. - Updated `Main.hs` to call exposed `readConfigs` with `defaultConfigPaths` (command line provided paths still take priority).

AlexeyRaga commented

2018-03-01 11:23:03 +01:00

(Migrated from github.com)

ping @lspitzner

lspitzner commented

2018-03-03 12:33:06 +01:00

(Migrated from github.com)

Hello Alexey!

Thanks for working on this issue!

I think the new functionality should be official and not just Internal, so this stuff should be also exported from Language.Haskell.Brittany.

Regarding semantics, this code contains some changes that I did not anticipate and which I do not understand yet.

Most importantly, it diverts from what was discussed in the HIE issue[1] by having defaultConfigPaths depend on the current working directory. Perhaps my suggestion over there was still a bit too short, because we may indeed also want a way to discover per-project configs from HIE; that is an aspect I had not considered and it is good that you point it out. But it still seems to me that exposing a method that depends on CWD is of no use to HIE. Please correct me if this is wrong or if you have a different usecase for which this makes sense.

Further, the semantics seem to have changed slightly:

Instead of searching for the first local brittany.yaml, we now consider and merge all brittany.yamls in cwd and its parents;
The per-user config now has higher precedence than project-specific config(s);
The default per-user config is only written if no brittany.yaml is found, while previously we could be sure it existed after running brittany once.

I am not sure which of these changes you intended (you certainly did not mention any of them in the PR or the commit-messages). From my point of view, 2 and 3 seem to be mistakes; and I am open about 1, but "if it aint broke dont fix it" so I wonder: which users want to have multiple (cascaded) per-project configs?

[1] https://github.com/haskell/haskell-ide-engine/issues/346

Hello Alexey! Thanks for working on this issue! I think the new functionality should be official and not just `Internal`, so this stuff should be also exported from `Language.Haskell.Brittany`. Regarding semantics, this code contains some changes that I did not anticipate and which I do not understand yet. Most importantly, it diverts from what was discussed in the HIE issue[1] by having `defaultConfigPaths` depend on the current working directory. Perhaps my suggestion over there was still a bit too short, because we may indeed also want a way to discover per-project configs from HIE; that is an aspect I had not considered and it is good that you point it out. But it still seems to me that exposing a method that depends on `CWD` is of no use to HIE. Please correct me if this is wrong or if you have a different usecase for which this makes sense. Further, the semantics seem to have changed slightly: 1. Instead of searching for the first local `brittany.yaml`, we now consider and merge all `brittany.yaml`s in cwd and its parents; 2. The per-user config now has higher precedence than project-specific config(s); 3. The default per-user config is only written if no brittany.yaml is found, while previously we could be sure it existed after running brittany once. I am not sure which of these changes you intended (you certainly did not mention any of them in the PR or the commit-messages). From my point of view, 2 and 3 seem to be mistakes; and I am open about 1, but "if it aint broke dont fix it" so I wonder: which users want to have multiple (cascaded) per-project configs? [1] https://github.com/haskell/haskell-ide-engine/issues/346

AlexeyRaga commented

2018-03-04 00:42:00 +01:00

(Migrated from github.com)

Hi Lennart!

Sorry, I have missed the exports.

As for CWD etc, I envisioned it to be a responsibility for an external tool to provide the config file. HIE (or other tools) could either use defaultConfigPaths or it could find a per-project file and pass it to readConfigs instead, or it can add it to the default list with a priority it thinks it is necessary, or it can take defaultConfigPaths and do any additions/filtering necessary.
I thought that it shouldn't be brittany dictating this to the tools, so I decided to try widening a bit your suggestion and allow this to be decided by the tools.

Changes in semantics:

Instead of searching for the first local brittany.yaml, we now consider and merge all brittany.yamls in cwd and its parents

It was intentional. It is how configs usually work and how GHCi works (or at least as it is supposed to work because there is currently a bug where "global" .ghci always overrides "local" settings).
We can change it if we don't want this behaviour in Brittany. Don't we?

The per-user config now has higher precedence than project-specific config(s)

I don't believe so, but I'll double check. My intention was for defaultConfigPaths to return "highest priority first", and then there is this line I inherited from the existing code:

paths -> readConfig `mapM` reverse paths

It reverses the list so local files configs will be at the end of the list overriding more global ones.

But as I said, I will double check.

The default per-user config is only written if no brittany.yaml is found, while previously we could be sure it existed after running brittany once.

I can change it. My intention was like "found something I can work with therefore don't change anything". But I think there is no harm in always writing a default config in case if it doesn't exist.

So I will:

Double check config files precedence (perhaps write a test)
Always write the default config as per your description.

Can we discuss merging files in a bit more details? Why don't we want the default config, which is automatically written and could be adjusted by user) to take effect for the settings that are not specified in a local config? I thought it would be just logical?

Hi Lennart! Sorry, I have missed the exports. As for CWD etc, I envisioned it to be a responsibility for an external tool to provide the config file. `HIE` (or other tools) could either use `defaultConfigPaths` or it could find a per-project file and pass it to `readConfigs` instead, or it can add it to the `default` list with a priority it thinks it is necessary, or it can take `defaultConfigPaths` and do any additions/filtering necessary. I thought that it shouldn't be `brittany` dictating this to the tools, so I decided to try widening a bit your suggestion and allow this to be decided by the tools. Changes in semantics: > Instead of searching for the first local `brittany.yaml`, we now consider and merge all `brittany.yaml`s in `cwd` and its parents It was intentional. It is how configs usually work and how GHCi works (or at least as it is supposed to work because there is currently a bug where "global" `.ghci` always overrides "local" settings). We can change it if we don't want this behaviour in Brittany. Don't we? > The per-user config now has higher precedence than project-specific config(s) I don't believe so, but I'll double check. My intention was for `defaultConfigPaths` to return "highest priority first", and then there is this line I inherited from the existing code: ``` paths -> readConfig `mapM` reverse paths ``` It reverses the list so local files configs will be at the end of the list overriding more global ones. But as I said, I will double check. > The default per-user config is only written if no brittany.yaml is found, while previously we could be sure it existed after running brittany once. I can change it. My intention was like "found something I can work with therefore don't change anything". But I think there is no harm in always writing a default config in case if it doesn't exist. So I will: 1) Double check config files precedence (perhaps write a test) 2) Always write the default config as per your description. Can we discuss merging files in a bit more details? Why don't we want the default config, which is automatically written and could be adjusted by user) to take effect for the settings that are not specified in a local config? I thought it would be just logical?

lspitzner commented

2018-03-04 18:51:42 +01:00

(Migrated from github.com)

For the last question: Yes, we certainly want to merge the different configs. I was talking about the case where there are multiple potential "per-project" configs, i.e. we are at /a/b/c/ and there is a brittany.yaml at both /a/b/ and at /a/b/c. Previously we considered only the closest one, this PR merges them (in some order i am not sure about, really.)

GHCi is not applicable as a reference for that aspect, because it only considers one "per-project" file: ./.ghci. (See https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/ghci.html#the-ghci-files)

The reverse indeed makes things confusing. Perhaps we should just remove it; it may not even be the expected thing to do when passing paths on the commandline, as later args normally to override earlier ones, but they don't with the reverse. And if you also refactor the create-if-missing logic to somewhere else, readConfigs could become short and neat.

Regarding CWD: You could not even use the current defaultConfigPaths for HIE. Because when it calls getCurrentDirectory, it asks where hie was started, which could be some random folder that contains no relevant brittany.yaml (in the best case). I am mildly certain that nothing calling getCurrentDirectory should be exposed, as it would only serve to confuse the user/the API.

So I will:

Double check config files precedence (perhaps write a test)

Always write the default config as per your description.

That sounds great! I'd prioritize some in-source comments containing examples over a test though; such system tests are annoying to write anyways.

For the last question: Yes, we certainly want to merge the different configs. I was talking about the case where there are multiple potential "per-project" configs, i.e. we are at `/a/b/c/` and there is a `brittany.yaml` at both `/a/b/` and at `/a/b/c`. Previously we considered only the closest one, this PR merges them (in some order i am not sure about, really.) GHCi is not applicable as a reference for that aspect, because it only considers one "per-project" file: `./.ghci`. (See https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/ghci.html#the-ghci-files) The `reverse` indeed makes things confusing. Perhaps we should just remove it; it may not even be the expected thing to do when passing paths on the commandline, as later args normally to override earlier ones, but they don't with the `reverse`. And if you also refactor the create-if-missing logic to somewhere else, `readConfigs` could become short and neat. Regarding `CWD`: You could not even use the current `defaultConfigPaths` for HIE. Because when it calls `getCurrentDirectory`, it asks _where hie was started_, which could be some random folder that contains no relevant `brittany.yaml` (in the best case). I am mildly certain that nothing calling `getCurrentDirectory` should be exposed, as it would only serve to confuse the user/the API. > So I will: > 1. Double check config files precedence (perhaps write a test) > 2. Always write the default config as per your description. That sounds great! I'd prioritize some in-source comments containing examples over a test though; such system tests are annoying to write anyways.

AlexeyRaga commented

2018-03-08 11:07:30 +01:00

(Migrated from github.com)

Lennart,

Please have a look at the updated PR and let me know if it is closer to what you have envisioned.

In this version it will:

Find exactly one user-level config.yaml. If there is none, the default one is created.
Find one "local" config file following the same algorithm as before (walking the tree until brittany.yaml is found). No more merges for "higher-level" configs.
By default brittany will still merge the "local" config file with the "user-level" one. Let me know if you don't want that, but I think it is a sensible way of overriding user's defaults.
There are two functions that are exposed: readConfigs and readConfigsWithUserConfig. Only the second one performs the merge I described above.

Lennart, Please have a look at the updated PR and let me know if it is closer to what you have envisioned. In this version it will: - Find exactly one user-level `config.yaml`. If there is none, the default one is created. - Find one "local" config file following the same algorithm as before (walking the tree until `brittany.yaml` is found). No more merges for "higher-level" configs. - By default `brittany` will still merge the "local" config file with the "user-level" one. Let me know if you don't want that, but I think it is a sensible way of overriding user's defaults. - There are two functions that are exposed: `readConfigs` and `readConfigsWithUserConfig`. Only the second one performs the merge I described above.

lspitzner commented

2018-03-13 23:03:30 +01:00

(Migrated from github.com)

@AlexeyRaga looks perfect! User and local config should indeed be merged in exactly this fashion (and if I am not mistaken it matches the previous behaviour).

Thanks!

I plan to include this in a new release in the next days.

@AlexeyRaga looks perfect! User and local config should indeed be merged in exactly this fashion (and if I am not mistaken it matches the previous behaviour). Thanks! I plan to include this in a new release in the next days.

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#121