Add Currencies library and get and parse rate dynamics

[suhanoves]

Игорь, привет! Меня зовут Евгений, я начинающий разработчик и это мой первый опыт, когда я пытаюсь законтрибьютить в опенсорс, поэтому если я что-то делаю не то, ты мне скажи, я буду исправляться.

Теперь к делу: я пользуюсь твоим модулем, но увидел, что на сайте Центробанка есть ещё интересные возможности, которые не покрыты pycbrf, а именно:

1. Библиотека всех иностранных валют с русскими и английскими названиями и кодами
2. Возможность получить по определённой валюте историю курсов на огромный период

Это привело меня к доработке твоего проекта, но его переосмысление немного ломает существующую совместимость. Сейчас поясню.

На текущий момент у тебя сведения о валюте существуют как аттрибуты экземпляра курса. Моя идея состоит в следующем:

1. Создаём библиотеку всех валют, вытягивая и распарсивая с сайте XML. Делаем данную библиотеку Singleton, чтобы она и все её изменения были доступны из любого экзепляра ставок. Это нужно потому, что в библиотеке валют удалены сильно устаревшие или заменённые валюты, которые иногда вылезают при запросе мультивальтного курса. Дополнительным плюсом является то, что валюта хранит в себе английское название и русское в именительном падеже.
2. Из экземпляра ExchangeRate убираем аттрибуты, привязанные к валюте. Остаётся валюта и параметры курса: дата, стоимость, номинал (с номиналом выяснились тоже интересные особенности, но об этом потом).
3. Экземпляр ExchangeRates делаем универсальным, он теперь может запрашивать и парсить как мультивалютный, так и мультивременной курсы в зависимости от запроса. Если валюта не задана, то значит мы хотим получить мультивалютный курс на дату. Если задана валюта - значит мы хотим получить мультивременной курс для определённой валюты.

Что мной сделано:

1. Библиотека в виде синглтона через мастеркласс. Хранение валют организовано в виде словаря, где для каждой валюты создаётся три ключа: id, num и rate. Я пытался организовать и через словарь + список (аналогично организацию словаря в современном питоне), но оказалось, что так потребление ресурсов даже выше. Подобная структура позволяет нам за O(1) находить любую существующую валюту по любому определяющему параметру, при этом независимо от того, как она передана: 'usd' 'USD' 'R01235' 'rR01235' '840' 840 c ведущим нулём и без. Валют всего 150, поэтому потребление ресурсов считаю незначительным.
2. Класс BetaExchangeRate - аналог твоего ExchangeRate, но чуть расширенный. Приставка Beta добавлена потому, что не на 100% сохраняется старый интерфейс, и не все тесты будут автоматически пройдены. Я считаю неправильным хренение параметров, относящиеся к одному курсу в библиотеке курсов. Поэтому BetaExchangeRate имеет дополнительные аттрибуты date_requested, date_received, dates_match, которые ранее у тебя относились к ExchangeRates. Для сохранения небольшой совместимости со старым интерфейсом созданы @property методы, которые получают данные из валюты курса, поэтому параметры валюты можно получить сразу обращаясь к экземпляру курса.
3. Класс BetaExchangeRates - аналог твоего ExchangeRates, который теперь умеет парсить в двух вариантах:
   • Если не задана валюта, мы ищем мультивальтный курс на сегодня (если не задана дата) или на заданную дату
   • Если валюта задана мы ищем курс валюты на сегодня (если не задана дата), на другую дату(если дата одна), или на курсы на диапазон дат, если заданы обе даты и валюта.
   • Доступ к экземпляру каждой ExchangeRate осуществляется либо по дате (если курс мультивременной), либо по любому параметру валюты, если курс мультивалютный. Тип BetaExchangeRates определяется по аттрибуту is_multicurrency
4. Добавлены кастомные исключения если не найдена валюта или курс, поэтому не будут проходить твои тесты, где при пустом значении у тебя возвращается None. Мне кажется правильным возбуждать исключения, как в словаре.

Во время парсинга обнаружились интересные моменты:

1. Иногда в мультивальтном курсе могут появиться валюты, которые уже не существуют в библиотеке. Так случилось со старыми белорусскими зайчиками. В этом случае мы добавляем неизвестную валюту в библиотеку и всё работает отлично (разве что оба имени будут на английском), но можно добавить ещё один запрос.
2. Номиналы валют в библиотеке и в курсах могут иногда отличатся. Написал в цетробанк, чтобы допилили библиотеку, но слабо в это верю. Поэтому для всех расчётов номиналы берутся из курса, а не валюты.
3. Обнаружил, что в библиотеке всегда валюта называется в именительном падеже,а в курсах может быть ещё и в родительном и с чем это связано непонятно. Поэтому имена берутся всегда из самой валюты.

Разумеется на всё написана подробная документация, везде расставлены аннотации типов и написано большое количество тестов.

Меня сильно смущает то, что старый интерфейс не на 100% совместим, поэтому жду от тебя твоей реакции и готов править код. P.S.: Если коммитов слишком много, могу слить их в один.

[idlesign]

Привет. Спасибо. После беглого осмотра комментарии общего толка:

1. Обратная совместимость, несомненно важна: люди частью просто не перейдут на новую абстракцию, а поддерживать две — сомнительное удовольствие. Поэтому, пожалуй, первое, что следует сделать — это вписать новые плюшки в существующие примитивы:
   • в ExchangeRate для самодостаточности действительно просится дата курса (только не запрошенная, а реальная);
   • туда же добавить атрибут currency, в котором будет расширенная информация о валюте;
   • а id, name, code, num пусть обращаются к атрибутам этой информации;
   • динамика тоже может быть полезна, но для неё лучше завести ExchangeRatesDynamics
2. Нужно подумать над целесообразностью внесения информации по наименованиям и кодам валют, ввиду существования ОКВ, доступного в частности из https://github.com/idlesign/ruopenrefs (данные там должны быть более полные, но минус в необходимости получения ключа для доступа к данным). Вообще штуки типа ОКВ и ОКСМ относительно редко меняются и небольшие, как вариант следует подумать над тем, чтобы их зашить, а не бегать в сеть.

[suhanoves]

1. Глянул ОКВ, вижу главный минус - это отсутствие кодов самого центробанка, а именно он нужен для запроса динамики ставок. Предлагаю оставить библиотеку, т.к.:
   • коды центробанка всё равно нужны
   • запрашивается она всего лишь один раз при инициализации любого из классов, а дальше работает один экземпляр
   • для её обновления с сохранением добавленных валют уже реализован метод .update
   • если появится более интересный вариант, реализацию класса можно поменять, а интерфейс останется неизменным
2. Что делать с исключениями? Для унификации так же возвращать None при остутствии данных или в ExchangeRatesDynamics поднимать исключения?
3. Да, хотел спросить и забыл, объекты даты почему были выбраны datetime, а не date? Ведь центробанк не отдаёт время, и курс актуален только на день. В ExchangeRatesDynamics в качестве ключей доступа к ExchangeRate будут выступать объекты даты. Что использовать date или datetime?

[suhanoves]

Игорь, глянь на текущую версию. Она полностью совместима с публичными интерфейсами твоей существующей версии. Все тесты, что были у тебя проходят на 100% (банки проходят на 66%, один пропускается, но так было и до этого, туда не лез) Ченжлог:

1. Добавлен класс Currency, представляющий собой валюту с её параметрами.
   • Хешируется для того, чтобы выступать ключём словаря
2. Добавлен класс CurrenciesLib, представляющий собой синглтон и загружающий список валют с сайта ЦБ РФ.
   • Имеет возможность ручного добавления валюты CurrenciesLib.add(), если её не было в библиотеке, но появилась в курсах.
   • Имеет возможность обновления библиотеки без затирания вручную добавленных валют CurrenciesLib.update
   • Представляет собой словарь с ключами из основных аттрибутов валюты (id, num, code), значение выступает экземпляр валюты.
   • Получение валюты по любому её ключевому параметру в любом регистре, str или num для num_code валюты
3. Переписан класс ExchangeRate:
   • Добавлен экземпляр валюты внутрь. Для обратной совместимости созданы property свойства для получения сведений валюты по аттрибутами самого ExchangeRate
4. Переписан класс ExchangeRates:
   • Переписана функциональность под новый ExchangeRate. Теперь аттрбут rates представляет собой словарь, ключём которого выступает валюта, что позволяет искать входящие ExchangeRate по любому ключевому параметру валюты.
   • В getitem возбуждаются кастомные исключения, но ловятся для обратной совместимости со старыми версиями. Возвращают None.
   • Если при получении курсов возникает валюта, отсутствующая в Библиотеке, она добавляется в Библиотеку.
   • Добавлен метод len
5. Добавлен класс ExchangeRateDynamics, представляет собой ExchangeRates для одной валюты с разными датами.
   • Аттрибут rates представляет собой словарь с ключём datetimes (сделан для унификации с ExchangeRate), значения ExchangeRate.
   • Способен искать как без аттрибутов даты(на сегодня), с одной датой (на дату), с двумя датами (диапазон)
   • Задан метод getitem, может принимать дату в строке, в виде объектов date или datetime
   • Добавлен метод len
   • В отличие от ExchangeRate при неверных параметрах возбуждает исключения, а не возвращает None, т.к. здесь обратная совместимость не требуется.
     6. Добавлены кастомные классы исключений WrongArguments, CurrencyNotExists, ExchangeRateNotExists
     7. Добавлен миксин FormatMixin для общих форматирований аттрибутов
     8. Добавлен миксин SingletonMeta - метакласс для создания синглтона
     9. Добавлена и актуализирована документация для всех классов и методов
     10. Добавлены дополнительные тесты для всех классов

[idlesign]

Спасибо. Постараюсь на неделе поглядеть реализацию.

[idlesign]

Дифф сейчас слишком сильно шумит. Я частично посмотрел, но ты перенеси новые сущности в конец файла, иначе изменения показываются не там, где они были.

[suhanoves]

Дифф сейчас слишком сильно шумит. Я частично посмотрел, но ты перенеси новые сущности в конец файла, иначе изменения показываются не там, где они были.

Currency сдвинуть вниз не получится, т.к. она используется в объявлении ExchangeRate. Currencies, ExchangeRateDynamics я менял местами, вставлял между, всё равно diff получается шумным, т.к. PyCharm цепляется не за те методы. Как вариант можно разнести по разным модулям.

[suhanoves]

Я постарался привести файлы чтобы diff лучше увидел изменения, но это не сильно помогло для rates.py (все остальные файлы стало сильно проще сравнивать с коммитом ad068241)

Ченжлог:

1. Вся документация приведена к формату Sphinx docstring (по мануалам тут и тут)
2. CurrenciesLib переименован в Currencies
3. banks.py приведён к версии из коммита ad068241
4. Исключение CurrencyNotExists переименовано в CurrencyNotFound
5. Исключение ExchangeRateNotExists переименовано в ExchangeRateNotFound
6. Переписан метод _format_num_code на f'{val}'.zfill(3)
7. Висящие строчки приведены в соответствие с рекомендациями
8. В классе Currencies в методе getitem заменены исключения при пустом аттрибуте на CurrencyNotFound
9. В классе Currencies в методе getitem переменная item_ переименована в item
10. В классе Currencies в методе _make_currency_pack переменная pack объявлена литералом {}
11. В классе Currencies в методе str убрано форматирование у времени обновления библиотеки.

На незакрытые вопросы жду ответов для дальнейшего приведения кода к требованиям.

[idlesign]

Что-то я закрутился, прошу прощения. На неделе постараюсь поглядеть и отписаться.

И ремарка: длинные описания обычно не пишут — и время берегут, и потому что часто видно из кода, что и как происходит. Но при обзоре могут, конечно, какие-то уточняющие вопросы появиться.

[idlesign]

Как вариант можно разнести по разным модулям.

Да, давай так попробуем. Потому, если что я пакет перенесём всё по курсам.

[suhanoves]

Т.к. библиотека Currencies сейчас инициализируется из константы, встаёт вопрос, стоит ли делать инициализацию ленивую загрузку?

Сейчас каждый вид курсов имеет аттрибут currencies_lib, который ведёт на единственный экземпляр библиотеки. Это дёшево, т.к. экземпляр один, и просто решает вопрос с добавлением валюты из курсов, которой нет в библиотке.

Не вижу с текущей реализацией необходимости в какой-то дополнителной ленивой загрузке, ведь больше в интернет мы не лезем. Что думаешь?

[idlesign]

Не вижу с текущей реализацией необходимости в какой-то дополнителной ленивой загрузке, ведь больше в интернет мы не лезем. Что думаешь?

Да, возможно, только непонятно, почему атрибут currencies_lib в объектах курсов. Например, можно рассмотреть вариант инициализации его на уровне модуля и присвоения атрибуту модуля прямо в currencies.py:

CURRENCIES = Currencies()

Только там ещё по коду есть недочёты с оформлением и с читаемостью дифа. Доразнеси, пожалуйста, по модулям: все новые сущности — в новые модули. Тогда можно будет ещё поглядеть с на сложность кода и на протечки абстракций. Возможно есть шанс снизить общую появившуюся сложность.

[suhanoves]

По максимуму разбил на отдельные модули, чтобы не пересекались новые и старые классы, но git всё равно шумит в rates.py на классе ExchangeRates, особвенно в методе __init__. Я как мог подвигал строчки кода, чтобы они соответствовали твоему, удалял и вновь добавлял docstring'и, но большего достичь не смог. Если будут вопросы - задавай

[idlesign]

Я добавил ещё пару комментариев. Разберём их, да я волью, потом сам подрихтую остальное, а то мы затянули это дело.

Я как мог подвигал строчки кода, чтобы они соответствовали твоему, удалял и вновь добавлял docstring'и, но большего достичь не смог.

Угу, похоже из-за строки документации класса, в которой описаны параметры из инициализатора. Давай уберём всё же. Можно минимальный пример использования оставить, но в целом примерам место в документации или readme.

[suhanoves]

Если что меня не беспокоит количество итераций, я на этом только учусь :) но понимаю, что для тебя это лишний напряг. Дай тогда ответ по поводу констант, надо ли сливать в одну, если надо я перепишу метод и солью. Остальное подправил.

[idlesign]

но понимаю, что для тебя это лишний напряг.

Не то чтобы напряг, скорее потому что описывать текстом дольше, чем подправить %)

[suhanoves]

Залил все последние исправления по замечаниям. Ты когда внесёшь исправления в коде после.перед мёрджем, маякни как-нибудь, пожалуйста. Я, во-первых, хочу поучиться и сделать для себя выводы. А во-вторых реализовать получение курсов металлов, как просили в соседней теме :)

[idlesign]

Спасибо, влил. Можно по истории поглядеть, какие я изменения внёс. Если будут вопросы по ним, можно прямо тут задать.