Skip to content

Conversation

@Erasiloco
Copy link
Contributor

string to DateTime

@Erasiloco Erasiloco self-assigned this Jul 16, 2020
@Erasiloco Erasiloco requested a review from bzaar July 16, 2020 16:41
Copy link
Member

@bzaar bzaar left a comment

Choose a reason for hiding this comment

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

См. замечания.

}

public string GetDate(string date, bool useOne)
public string GetDate(DateTime dateTime, bool useOne)
Copy link
Member

Choose a reason for hiding this comment

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

Уже была выпущена версия с этим методом. Если его изменить, то поломается обратная совместимость. Лучше добавить новый метод (перегрузку) с параметром DateTime, а этот пометить как Obsolete.

using (var client = _newClient())
{
client.AddParam("date", date);
client.AddParam("date", dateTime.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

Нужно отформатировать дату. Иначе можешь получить что-нибудь вроде "21. Juni 2020" в зависимости от региональных настроек ОС.

@Erasiloco Erasiloco requested a review from bzaar July 20, 2020 13:49
Copy link
Member

@bzaar bzaar left a comment

Choose a reason for hiding this comment

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

См. комментарии.

}
catch (BadRequestException e) when (e.ErrorCode == 19)
{
throw new OutOfRangeException(nameof(n));
Copy link
Member

Choose a reason for hiding this comment

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

Какое значение нужно передать в метод GetCardinal, чтобы сервис вернул ответ с кодом 19?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OutOfRangeException будем возвращать если n больше int.Max или меньше int.Min.

Copy link
Member

Choose a reason for hiding this comment

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

Зачем? Сервис умеет строить пропись для всех значений типа long.

class BadRequestException : Exception
{
public int Status { get; }
public int ErrorCode { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Все хорошо, только нужно где-то установить значение ErrorCode. Думаю, лучше это сделать в конструкторе BadRequestException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок

@Erasiloco Erasiloco requested a review from bzaar July 20, 2020 22:13
Copy link
Member

@bzaar bzaar left a comment

Choose a reason for hiding this comment

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

См. замечания и прошу для каждого нового метода добавить XML comments и указать исключения, которые он может выбрасывать. Вот так.

}
catch (BadRequestException e) when (e.ErrorCode == 19)
{
throw new OutOfRangeException(nameof(n));
Copy link
Member

Choose a reason for hiding this comment

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

Зачем? Сервис умеет строить пропись для всех значений типа long.

public int Status { get; }
public int ErrorCode { get; }

public BadRequestException(int status, int errorCode)
Copy link
Member

Choose a reason for hiding this comment

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

Конструктор не используется, следовательно, свойство ErrorCode всегда будет 0.

if (exc.Response is HttpWebResponse httpWebResponse)
{
int status = (int)httpWebResponse.StatusCode;
int errorCode = int.Parse(httpWebResponse.Headers.Get("Error-Code"));
Copy link
Member

Choose a reason for hiding this comment

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

ErrorCode находится в теле запроса, а не в заголовке.

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