Implemented support for dayOfYear #1
Conversation
Implemented support for dayOfYear.
|
This is BTW as agreed on in schyntax/schyntax#14. |
|
The error is due to the new test-suite that is not yet in the generated test.json file. |
|
You can include the |
|
I've only taken a quick glance at the code. Looks like it's on the right track, but make sure you're using spaces, not tabs. I actually prefer tabs, but all of our stuff at Stack Overflow uses spaces, so that's what I use in C# libraries. Should be pretty easy to spot all the places you inserted tabs, and if you could fix those up, I'll take a closer look at the PR. Thanks. |
|
Cool. I'll do a check on this. I recently reinstalled my computer so it probably have returned to defaults. So, I'll change to spaces. We hade the religious discussion in the office a while back. I too favor tabs, but it seems like instead of the world fixing their use of tabs (only tab to indent-level, then spaces) they all took the easy way out and decided to go for spaces... I'll do some edits and then add the test.json too. BTW: Maybe it could be a solution to fetch the test.json from the Schyntax project as a submodule? Not sure if it is possible to get only one of the files in the sub-repo though, but that could be an option - perhaps. Or - have the test.json file distributed as a separate NuGet package, and then have ports depend on the Sxhyntax.Test.Json package |
- Whitespace for changed code was with tabs, should be with spaces. - Also added the compiled test.json result from the base Schybtax project that contains the new tests for dayOfYear.
Implement dayofyear
|
Maybe I should stop creating branches on my fork, or send the PR directly from the branch... I see that there are some extra commits because of this. Also, my master will then diverge from the parent upstream repo... |
|
I generally do pull requests from a branch, not master on a fork. You should still have been able to do fast-forward merges though, which wouldn't generate extra merge commits. Looks like you merged via pull requests though, which always generates a merge commit. |
|
Yeah. Long time since I did any GitHub contribs, evidently.. Well, learning by doing :/ |
| @@ -58,6 +58,14 @@ public class IrGroup | |||
| public bool HasDaysOfMonthExcluded => _daysOfMonthExcluded != null && _daysOfMonthExcluded.Count > 0; | |||
| public List<IrIntegerRange> DaysOfMonthExcluded => _daysOfMonthExcluded ?? (_daysOfMonthExcluded = new List<IrIntegerRange>()); | |||
|
|
|||
| private List<IrIntegerRange> _daysOfYear; | |||
| public bool HasDaysOfYear => _daysOfYear != null && _daysOfYear.Count > 0; | |||
| public List<IrIntegerRange> DaysOfYear => _daysOfYear ?? (this._daysOfYear = new List<IrIntegerRange>()); | |||
There was a problem hiding this comment.
You can drop the this. here. I'm assuming that was a mistake since you're not using it anywhere else.
There was a problem hiding this comment.
True. Using this instead of _ for private fields is the convention used in my company. Default StyleCop. So, this is the default when I add code. I'll clean up.
There was a problem hiding this comment.
Yep, I totally get that everyone is used to different styles. Just trying to stay consistent within a given codebase.
|
Other than the nitpicks, this looks great. I'd like to see those fixes, and then have all of the commits squashed into one (via |
|
Closing this as it is replaced by PR #3. |
The tests should be committed as part of the Schyntax project I take it. But, what come first? Tests for Schyntax or the CS-Schyntax iomplementation. Should I create a PR for Schyntax with the new tests?