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

Transfer of route paths from hard-code to config #4829

Closed
wants to merge 1 commit into from

Conversation

katiefenn
Copy link

issue #4519

  • Removal of hard-coded route paths from controller file
  • Creation of route config in example config file
  • New aliases for other rss paths
  • Substitution of hard-coded paths in response context creation
  • Added unit tests for new routes and aliases

issue TryGhost#4519
- Removal of hard-coded route paths from controller file
- Creation of route config in example config file
- New aliases for other rss paths
- Substitution of hard-coded paths in response context creation
- Added unit tests for new routes and aliases
@katiefenn
Copy link
Author

As I've made large changes to the config.js file, I'm happy to take some feedback on how it could be tweaked or improved.

@kfenn-inviqa
Copy link

I've seen the broken build and will investigate when I can. I ran grunt validate before submitting, but didn't observe this problem. Will be in-touch.

@halfdan
Copy link
Contributor

halfdan commented Jan 20, 2015

Hi @katiefenn,
I haven't looked at your PR in depth yet so I just have a note for the config: We usually try to keep the example config very slim and don't include all the defaults there. Instead they live at https://github.com/TryGhost/Ghost/blob/master/core/server/config/index.js#L149-L195 but can be overridden when used in the config.js. I think that the options you added would much better be placed there in order to not clutter up the example config.

@ErisDS
Copy link
Member

ErisDS commented Jan 20, 2015

Hi @katiefenn :)

Reading back through the issue, I see that I wasn't clear on my intentions with this refactor. The idea was to rip out the english words from the routes: the words tag, author, page, and feed which a user might wish to change either to a different thing (like genre instead of tag) or to translate - as opposed to ripping out all of the routes and making them customisable.

The idea in the end is that there will be a setting where I can change 'tag' to 'genre' or similar, but for now, there was no need to make them configurable, just doing the hard part of ensuring the routes always work.

The big difference between putting something in our config (in which case @halfdan's comment about keeping them internally rather than in the example file is correct) vs a setting, is that a setting can be changed on-the-fly, whereas config requires a restart of Ghost. So with config you can guarantee that the strings are loaded before the routes, but with a setting you can't.

I'm not 100% which route we'd go down here - I think changing the word for a tag is likely to want to be a setting, and I have no idea how translations will be loaded.

That was the reason for leaving this refactor at the point where the strings are refactored out into an internal place, so at least changing them is done in one place - because there are other places than just the routes where those strings are hard-coded E.g: https://github.com/TryGhost/Ghost/blob/master/core/server/config/url.js#L125

@katiefenn katiefenn closed this Jan 30, 2015
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

4 participants