danikf / tik4net

Manage mikrotik routers with .NET C# code via ADO.NET like API or enjoy O/R mapper like highlevel api.
Apache License 2.0
174 stars 93 forks source link

Bugfix and spelling fix #6

Closed D-Bullock closed 8 years ago

D-Bullock commented 8 years ago

Sorry about the formatting issues with my last pull request.

danikf commented 8 years ago

Hi,

I have a litle problem with your change private TikEntityPropertyAccessor GetPropertyDescriptor(string fieldName) { return _properties.ContainsKey(fieldName) ? _properties[fieldName] : null;

The main reason is, that GetXXXX methods should always return not null values. If they don't, you will get another (ReferenceNotSet) exception in calling method becase typical usage is: GetXXXXX(...).UseIt(....). If method could return null (if not found), the name of the metod should be FindXXXX.

In this casse I will prefer to throw more specific exception (Property '...' is missing.)

D

D-Bullock commented 8 years ago

Ah, the reason I made the change was because of Delete in TikConnectionExtensions.cs:367

public static void Delete<TEntity>(this ITikConnection connection, TEntity entity)
{
    var metadata = TikEntityMetadataCache.GetMetadata<TEntity>();
    EnsureNotReadonlyEntity(metadata);
    string id = metadata.IdProperty.GetEntityValue(entity);
    if (string.IsNullOrEmpty(id))
        throw new ArgumentException("Entity has no .id (entity is not loaded from mikrotik router)", "entity");

    ITikCommand cmd = connection.CreateCommandAndParameters(metadata.EntityPath + "/remove", TikCommandParameterFormat.NameValue,
        TikSpecialProperties.Id, id);
    cmd.ExecuteNonQuery();
}

It checks for a null ID so I assumed it was a bug. Sorry for pushing bad code.

danikf commented 8 years ago

Hi - your comment shows me, that actual implementation was a little bit wrong - thank you. I have updated sources to be more consistent (/// documentation and real implementation).

Thank you, D