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

Rename custom/conf/app.ini.sample to custom/conf/app.example.ini for better syntax light on editor #11926

Merged
merged 4 commits into from
Jun 18, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 17, 2020

As title.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 17, 2020
@silverwind
Copy link
Member

Need to update docs and .gitignore.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 17, 2020
@silverwind
Copy link
Member

While we're renaming this, I'd prefer app.example.ini over app.sample.ini.

@mrsdizzie
Copy link
Member

Maybe app.default.ini? since these aren't just examples/samples but actually contain the default values in use for everything (and makes it obvious to look there for all default settings)

@silverwind
Copy link
Member

silverwind commented Jun 17, 2020

Maybe app.default.ini

I think the general expectance is that users copy the file to the actual config file and in this case, I think "example" fits better than "default" as I don't think gitea can run or do anything useful without a config file (or the equivalent env vars).

@zeripath
Copy link
Contributor

Users really should not copy the app.ini.sample as the default. We really should not encourage that!

If we want to provide them with a basic app.ini we should do that.

@silverwind
Copy link
Member

Users really should not copy the app.ini.sample as the default

Why not? It gets you a full set of options. It's a perfect starting point imho :)

@jolheiser
Copy link
Member

I've always just gone with the minimal one generated with installation and then added settings as needed.

I don't like the clutter of a bunch of settings that are all default if I can avoid it, if only because our config is massive.

@silverwind
Copy link
Member

I see, a minimal approach can be viable as well so fine with naming it "default" as well.

@zeripath
Copy link
Contributor

  • It hardcodes all sorts of currently default options into your app.ini preventing you from benefiting from updates to defaults.
  • There's examples in there that don't represent any real config - e.g. in the logging section.
  • It prevents settings which change the default setting of other settings from working. (I'm not sure I can immediately think of a case but I bet there is a case.)
  • It makes Gitea appear harder to configure than it really is.

In most configuration less is more.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 17, 2020
@6543
Copy link
Member

6543 commented Jun 17, 2020

it got my lgtm - like the idear but it will need some update here:

docs/content/page/index.en-us.md
78:        - [app.ini](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)

docs/content/page/index.fr-fr.md
73:        - Voir [ici](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)

docs/content/doc/advanced/customizing-gitea.en-us.md
38:- [Complete List](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)

docs/content/doc/advanced/customizing-gitea.zh-cn.md
25:- [完整配置清单](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)

docs/content/doc/advanced/config-cheat-sheet.zh-cn.md
18:这是针对Gitea配置文件的说明,你可以了解Gitea的强大配置。需要说明的是,你的所有改变请修改 `custom/conf/app.ini` 文件而不是源文件。所有默认值可以通过 [app.ini.sample](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample) 查看到。如果你发现 `%(X)s` 这
样的内容,请查看 [ini](https://github.com/go-ini/ini/#recursive-values) 这里的说明。标注了 :exclamation: 的配置项表明除非你真的理解这个配置项的意义,否则最好使用默认值。

docs/content/doc/advanced/config-cheat-sheet.en-us.md
26:accurately recorded in [app.ini.sample](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)
466:- `ALLOWED_TYPES`: **see app.ini.sample**: Allowed MIME types, e.g. `image/jpeg|image/png`.

docs/content/doc/features/authentication.en-us.md
99:    app.ini](https://github.com/go-gitea/gitea/blob/master/custom/conf/app.ini.sample)

@lunny lunny changed the title Rename custom/conf/app.ini.sample to custom/conf/app.sample.ini for better syntax light on editor Rename custom/conf/app.ini.sample to custom/conf/app.example.ini for better syntax light on editor Jun 18, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 18, 2020
@techknowlogick techknowlogick added this to the 1.13.0 milestone Jun 18, 2020
@techknowlogick
Copy link
Member

ping LG-TM

@techknowlogick techknowlogick merged commit 03ba974 into go-gitea:master Jun 18, 2020
@lunny lunny deleted the lunny/rename_sample branch June 18, 2020 03:33
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
…better syntax light on editor (go-gitea#11926)

* Rename custom/conf/app.ini.sample to custom/conf/app.sample.ini for better syntax light on editor

* rename to app.example.ini

* per @6543 's comment, update all references on docs

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants