Add GistConfig class #68
Conversation
| { | ||
| Arr::set($this->settings, $offset, null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Expected 1 newline at end of file; 0 found
|
Updated the relevant Nitpick CI issues. |
|
@JacobBennett I'm gonna have to make it easy to ignore things like test files I think, heh... Wouldn't worry about anything that isn't main source code, definitely prefer snake_case test names, don't care about namespaces in migrations, etc. |
|
OK, I deleted all the useless nitpick errors. :) I'll try to review this PR ASAP; thanks so much @JacobBennett! |
|
@adamwathan yeah I was betting the snake case was the preferred convention. First time I've seen nitpick. Really cool tool. @mattstauffer no prob. Glad to help in any way I can. Would love to help implement other ideas regarding the published at date as well. |
| /** | ||
| * @var array | ||
| */ | ||
| private $default_settings = array( |
There was a problem hiding this comment.
We can use short array syntax here and on :21
There was a problem hiding this comment.
I think we've done camelCase everywhere else in the app, both for properties and for inline variables
There was a problem hiding this comment.
Does this include the keys in the defaultSettings array?
for example:
$defaultSettings = [
'published',
'publishedOn',
'preview',
];
|
I made some notes; With these changes, this looks great. As a side note, we'll need to add some directions on how to use all this stuff, but that'll be a separate issue/PR. Thanks!! |
|
I'll get to work on these changes and have them done hopefully soon. Some great insights! I've never actually had anyone do a "code review" for me. Pretty much a one man show at my current job. Just goes to show the incredible value to be gained from having another set of eyes look over your code. Thanks. |
|
@mattstauffer so I made all the changes regarding new lines, short arrays, etc. The only thing left, which I'm thinking of adding in another PR is the date issue for the published_on setting. My thoughts for the next PR would include the following:
If you're okay with that, we can probably merge and close out this PR. Let me know. |
|
Hey @mattstauffer, have intentionally held off on being annoying and asking about this because I know you are likely slammed at the moment, but wanted to check and see if there was anything you needed me to do or if you needed any further explanation on this PR. Essentially pulling the PR in will allow authors to set a custom preview, thats it at this point. Any other hooks are strictly for future dev. |
|
@JacobBennett Thanks for the ping! Got back from vacation yesterday. Let me check it over again. :) |
|
@JacobBennett Hey, this is is all great, but I'd love for us to avoid committing anything to master that shows the MM-DD-YYYY date to avoid confusion. I know you want to save it for a separate PR, but can you either drop the date stuff from this one, or just do that separate PR against this branch before we merge this branch to master? I don't want to delay this launch because it's great work but I don't want to merge in anything with the wrong date format. :) Thanks! |
chore: update Exception to catch correct type chore: PSR-2 cleanup
|
@mattstauffer The date(s) in the YML file will now be parsed into To do so, the YML key to be used as a date must be specified in the GistConfig The value of said date key must be in the format specified in http://symfony.com/legacy/doc/reference/1_3/en/02-YAML#chapter_02_sub_dates, which is in Since the dates are being parsed to a |
|
Hey @mattstauffer, did you happen to see my last ping on this one? Think we should have all the changes set to go for this. |
|
@JacobBennett I hadn't seen it. Looking at it now. Thanks! |
|
@JacobBennett with the extra lines dropped, this looks fantastic. Just push up those changes (sorry to nit pick!) and it's ready to go--I'll finally merge this sucker. Thanks man! |
|
Didn't get a chance to make those changes before you merged, but thanks for merging it in! Whew! Now onto some other issues and pull requests :) |
Allows an author to add a gistlog.yml to their gist and have the following values pulled into a config file for the gist:
The main goal of this PR is to allow authors to customize the preview text that is used for their gists on the author page. The other two config items will be used in future development to allow more fine grained control over the presentation and publishing of their posts.