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

Feature/orcomp 644 #278

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Feature/orcomp 644 #278

wants to merge 3 commits into from

Conversation

Dmitry-VF
Copy link
Contributor

Description of Change

As noted by @mkhomutov , in the current implementation, the context is actually not really the context.
It should be a returning value of Parse() method.

Issues Resolved

  • fixes #

API Changes

None

Platforms Affected

  • All
  • WPF
  • UWP
  • iOS
  • Android

Behavioral Changes

None

Testing Procedure

PR Checklist

  • I have included examples or tests
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@Dmitry-VF Dmitry-VF self-assigned this Nov 8, 2021
@GeertvanHorrik
Copy link
Member

I think we should consider using System.CommandLine instead?

Copy link
Member

@mkhomutov mkhomutov left a comment

Choose a reason for hiding this comment

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

please write the tests for the new method and read my comments

{
using Catel.Data;

public interface IResult : IContext
Copy link
Member

Choose a reason for hiding this comment

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

one of the points of the issue is to split IContext into IResult and ICommandLineParsingContext
it means IResult should not be inherited from IContext

I would extract two different interfaces from IContext: ICommandLineParsingContext and something like ICommandLineParsingResult and do something like that:

[ObsoleteEx]
public interface IContext : ICommandLineParsingContext, ICommandLineParsingResult
{
    // put obsolete props and methods here
}

public interface IValidatedResult : ICommandLineParsingResult
{
   IValidationContext ValidationContext { get; }
}

{
using Catel.Data;

public interface IValidatedResult : IContext
Copy link
Member

Choose a reason for hiding this comment

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

read the comments for the IResult


public List<char> QuoteSplitCharacters { get; set; }

public void Finish()
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why we need it

using System;
using System.Reflection;

internal static class PropertyHelper
Copy link
Member

Choose a reason for hiding this comment

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

rename the class to ObjectExtensions

@mkhomutov
Copy link
Member

I think we should consider using System.CommandLine instead?

good point

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 16, 2022
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