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

TryParse overload #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Atrejoe
Copy link

@Atrejoe Atrejoe commented Nov 1, 2019

Added overload more similar to common TryParse implementations

@Atrejoe
Copy link
Author

Atrejoe commented Nov 1, 2019

I implemented your suggestion of here: #11 (comment)

Instead of having to do:

bool IsCronExpressionValid(string expression) =>
    CrontabSchedule.TryParse(expression) != null;

one can now also call:

if(CrontabSchedule.TryParse(expression, var schedule))
    //Do something with the schedule

or for validation only

if (CrontabSchedule.TryParse(expression, var _))
    //your logic here

@Atrejoe
Copy link
Author

Atrejoe commented Nov 1, 2019

I think the AppVeyor build failure may not be due to my commit.

@atifaziz
Copy link
Owner

I think the AppVeyor build failure may not be due to my commit.

Fixed with da31efd.

@rshillington
Copy link

Any chance of getting the PR merged?

Added overload more similar to common TryParse implementations
@Atrejoe Atrejoe force-pushed the TryParseoverload branch from da31efd to 512c3b8 Compare May 14, 2024 19:03
@Atrejoe Atrejoe force-pushed the TryParseoverload branch from 0927da4 to 034bec1 Compare May 14, 2024 19:33
Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.73%. Comparing base (dfea895) to head (034bec1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   88.37%   87.73%   -0.65%     
==========================================
  Files           5        5              
  Lines         430      432       +2     
  Branches        0       87      +87     
==========================================
- Hits          380      379       -1     
+ Misses         50       49       -1     
- Partials        0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Atrejoe
Copy link
Author

Atrejoe commented May 14, 2024

I'm trying to get a necromancer badge: revived a nearly 5-year old PR just for kicks.

I still believe TryParse should return bool with a value out param though :)
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern

Copy link
Owner

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to get a necromancer badge: revived a nearly 5-year old PR just for kicks.

Right, let's try to get this, and boy, thanks for your patience. 😅

I've had an initial look and added my comments.

Comment on lines +103 to +104
public static bool TryParse(string expression, out CrontabSchedule? schedule)
=> (schedule = TryParse(expression, null))!=null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting inconsistencies:

Suggested change
public static bool TryParse(string expression, out CrontabSchedule? schedule)
=> (schedule = TryParse(expression, null))!=null;
public static bool TryParse(string expression, out CrontabSchedule? schedule) =>
(schedule = TryParse(expression, null)) != null;

Comment on lines +106 to +107
public static bool TryParse(string expression, ParseOptions options, out CrontabSchedule? schedule)
=> (schedule = TryParse(expression, options)) != null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting inconsistencies and options needs to be nullable to match the type of the argument of the overload being delegated to:

Suggested change
public static bool TryParse(string expression, ParseOptions options, out CrontabSchedule? schedule)
=> (schedule = TryParse(expression, options)) != null;
public static bool TryParse(string expression, ParseOptions? options, out CrontabSchedule? schedule) =>
(schedule = TryParse(expression, options)) != null;

public static bool TryParse(string expression, out CrontabSchedule? schedule)
=> (schedule = TryParse(expression, null))!=null;

public static bool TryParse(string expression, ParseOptions options, out CrontabSchedule? schedule)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schedule parameter should be decorate with NotNullWhen(true) for correct nullability analysis.

You'll probably need to bring in PolySharp since NotNullWhenAttribute isn't defined for the frameworks that NCrontab targets. For .NET Framework 3.5, you may need conditional compilation too.

Assert.That(CrontabSchedule.TryParse(string.Empty), Is.Null);
Assert.False(CrontabSchedule.TryParse(string.Empty, out var _));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of the tests, use Assert.That:

Suggested change
Assert.False(CrontabSchedule.TryParse(string.Empty, out var _));
Assert.That(CrontabSchedule.TryParse(string.Empty, out var result), Is.False);
Assert.That(result, Is.Null);

Also, checking that result is indeed null would be more thorough.

Assert.That(CrontabSchedule.TryParse(null!), Is.Null);
Assert.False(CrontabSchedule.TryParse(string.Empty, out var _));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

  • The first argument to TryParse here should be null, not string.Empty.
  • For consistency with the rest of the tests, use Assert.That.
  • Testing that result is indeed null would be more thorough.
Suggested change
Assert.False(CrontabSchedule.TryParse(string.Empty, out var _));
Assert.That(CrontabSchedule.TryParse(null!, out var result), Is.False);
Assert.That(result, Is.Null);

Comment on lines +352 to +355
Assert.False(CrontabSchedule.TryParse(expression, new ParseOptions
{
IncludingSeconds = includingSeconds
}, out var _));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of the tests, use Assert.That:

Suggested change
Assert.False(CrontabSchedule.TryParse(expression, new ParseOptions
{
IncludingSeconds = includingSeconds
}, out var _));
Assert.That(CrontabSchedule.TryParse(expression, new ParseOptions
{
IncludingSeconds = includingSeconds
}, out _), Is.False);

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.

3 participants