suggestion (bug_risk): Connection timeout units differ from IConnectionInfo, which may cause confusion and subtle bugs.
In ILdapConnectionAdapter, ConnectionTimeout is an int in milliseconds, while IConnectionInfo uses a short in seconds. This discrepancy can lead to incorrect conversions or loss of precision when mapping between them. Please either standardize the unit across both interfaces or make the unit explicit in the property names (e.g., ConnectionTimeoutMs / ConnectionTimeoutSeconds).
Suggested implementation:
namespace Bitai.LDAPHelper.LdapAdapters;
/// <summary>
/// Defines the operations required to manage and use an LDAP connection.
/// </summary>
public interface ILdapConnectionAdapter : IDisposable
{
/// <summary>
/// Gets or sets the connection timeout in seconds.
/// This property uses the same unit and underlying type as <c>IConnectionInfo.ConnectionTimeout</c>
/// to avoid confusion and conversion issues.
/// </summary>
short ConnectionTimeoutSeconds { get; set; }
- Update all implementations of
ILdapConnectionAdapter to replace the old ConnectionTimeout property (milliseconds) with the new short ConnectionTimeoutSeconds property, including backing fields and any related logic.
- Adjust any mapping code between
IConnectionInfo and ILdapConnectionAdapter to use ConnectionTimeoutSeconds directly without converting units.
- Fix any consumers that still expect a millisecond-based timeout; if millisecond precision is needed in some places, compute it locally via
TimeSpan.FromSeconds(ConnectionTimeoutSeconds) or ConnectionTimeoutSeconds * 1000.
- If the old
ConnectionTimeout property was public and already in use, consider marking it as obsolete (with [Obsolete]) or removing it entirely, depending on your versioning and compatibility requirements.
Originally posted by @sourcery-ai[bot] in #30 (comment)
suggestion (bug_risk): Connection timeout units differ from IConnectionInfo, which may cause confusion and subtle bugs.
In
ILdapConnectionAdapter,ConnectionTimeoutis anintin milliseconds, whileIConnectionInfouses ashortin seconds. This discrepancy can lead to incorrect conversions or loss of precision when mapping between them. Please either standardize the unit across both interfaces or make the unit explicit in the property names (e.g.,ConnectionTimeoutMs/ConnectionTimeoutSeconds).Suggested implementation:
ILdapConnectionAdapterto replace the oldConnectionTimeoutproperty (milliseconds) with the newshort ConnectionTimeoutSecondsproperty, including backing fields and any related logic.IConnectionInfoandILdapConnectionAdapterto useConnectionTimeoutSecondsdirectly without converting units.TimeSpan.FromSeconds(ConnectionTimeoutSeconds)orConnectionTimeoutSeconds * 1000.ConnectionTimeoutproperty was public and already in use, consider marking it as obsolete (with[Obsolete]) or removing it entirely, depending on your versioning and compatibility requirements.Originally posted by @sourcery-ai[bot] in #30 (comment)