rename factory methods #60
Conversation
Signed-off-by: carlculator <info@alexander-schulz.eu>
timeseriesx/base/timestamp_series.py
Outdated
| return TimestampSeries.create_null_series(start, end, freq, unit=unit, | ||
| time_zone=time_zone) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
Using classmethod instead of staticmethod allows avoiding hardcoding the class name for inheritance.
There was a problem hiding this comment.
good point. changed this
There was a problem hiding this comment.
For backward compatibility, the old (now deprecated) methods must remain decorated with staticmethod.
timeseriesx/base/timestamp_series.py
Outdated
| unit=unit, time_zone=time_zone) | ||
|
|
||
| @staticmethod | ||
| def create_constant_series(start, end, value, freq, unit=None, time_zone='infer'): |
There was a problem hiding this comment.
start, end and perhaps value are readable enough when positional, but freq and especially unit and time_zone are better given as keywords only. This is also how the mixins require it. As such, I would change the definition to:
def create_constant_series(start, end, value, *, freq, unit=None, time_zone="infer"): ...There was a problem hiding this comment.
I like this idea of keywords only. I did not apply it elsewhere in the code, so maybe it would be better to overhaul this globally in one go? What do you think? I don't understand how the mixins affect this factory method?
There was a problem hiding this comment.
I agree keywords only and positional only arguments make for cleaner interfaces, but introducing them after the fact comes with a lot of backward incompatibility. Perhaps a good issue to tackle before a major release.
The mixins are already implicitly keyword only because they only work when the arguments like freq are given as keywords. This is because they get their data only from **kwargs and never from *args (which would be tricky). Try running the tests after changing the keyword arguments of the super constructor call in TimestampSeries.__init__ to positional arguments to see that it does not work. That is not strictly related to this factory method, but they both create instances from very similar arguments, hence the comparison.
timeseriesx/base/timestamp_series.py
Outdated
| warnings.warn("this method was renamed, and will be deprecated in a future " | ||
| "version, please use `create_constant_series` instead", | ||
| category=DeprecationWarning) | ||
| return TimestampSeries.create_constant_series(start, end, value, freq, | ||
| unit=unit, time_zone=time_zone) |
There was a problem hiding this comment.
Even if #53 is not yet merged, you can already start using Black (which would be easier if it were merged).
Signed-off-by: carlculator <info@alexander-schulz.eu>
Signed-off-by: carlculator <info@alexander-schulz.eu>
deprecate
TimestampSeries.create_null_timeseriesandTimestampSeries.create_constant_timeseries, rename toTimestampSeries.create_null_seriesandTimestampSeries.create_constant_series.motivation:
the created series is a Timestamp-Series, a Time-Series and a Series. The most explicit name would be
create_constant_timestampseries. Since this is already provided by the namespace of the class, I'd go for the most generic version, which is Series.