search

status-im / nim-stew

stew is collection of utilities, std library extensions and budding libraries that are frequently used at Status, but are too small to deserve their own git repository.
139 stars 18 forks source link

Add API call from a (slight) future 1.6 branch with when guards to #173

Closed c-blake closed 1 year ago

c-blake commented 1 year ago

Add API call from a (slight) future 1.6 branch with when guards to deactivate as per claim here

https://github.com/status-im/nim-codex/pull/346#issuecomment-1432073926

that this (or something very near it) is where it belongs.

arnetheduck commented 1 year ago

hm, I'm not sure where the name parse.nim1 is coming from, but this functionality is in parseutils, why not put it in stew/shims/parseutils? ideally we can replace std/ with stew/shims and the backwards compat magically just works

c-blake commented 1 year ago

I agree stew/shims/parseutils would be a better name for that module. There was already a parseHex (also from std/parseutils) in a parse.nim in stew/shims. This was added by @mratsim in May, 2019 according to git log --follow parse.nim.

To me it was a question of "which to be consistent with" - existing history/micro-practice (just parse.parseHex) or macro-practice (like the other stew/shims modulenames). Renaming things after they are placed is usually disruptive which "broke the tie" for me, but if you are passionate about it, a stew/shims/parseutils could also always import/export a not-going-anywhere parse with an old-name deprecation to get all of the above.

On the module name topic, I also do not see or recall any stddefects in upstream Nim stdlib. That should perhaps be called stew/shims/exceptions?

Anyway, as mentioned in the comment of this patch, 1.6.12 will have this backported & may be mere weeks away (if past release cadence is any guide), making this is largely a 1.2/1.4 series compatibility question -- which I'm also not sure matters anymore.

arnetheduck commented 1 year ago

Lol, 2019, those were the days ;) can't find any parse.nim, even in the earlier versions so it might be that we were trying to land a new module at the time but thought better of it.

Anyway, I'd go for parseutils for upstream compatibility (which clearly is the authoritative source in this case) + import/export for compatiblity - agree on stddefects as well, that it should be named different with a backwards-compat import/export module - this happens on occasion with nim-stew, ie we try an idea and find out it was bad after all, like https://github.com/status-im/nim-stew/blob/master/stew/ranges/ptr_arith.nim for example.

arnetheduck commented 1 year ago

re 1.2, if we don't have to break compat, I'd rather see we keep supporting it for a while (ie assuming cost is low) - ie there may be community projects out there using stew / other code, so we'd rather not break such project until a concrete reason appears.

eventually, we'll start supporting certain features and libraries in 1.6-mode only with 1.2 remaining partially supported / best effort, and one fine day we'll simply axe 1.2 completely. We're not there yet though, specially not for libraries as close to the core of the dependency tree as stew.

c-blake commented 1 year ago

Ok. Module renamed as suggested. If all these tests pass, then I will merge and back to 1.2 (or further) can access these calls unless I hear otherwise.

If you want to rename shims/stddefects, you can in a separate PR. That is at least used other places in stew (ranges/[typedranges, stackarrays])

c-blake commented 1 year ago

Well, all the tests passed, but someone with write access (which I do not have) has to do the merge.

arnetheduck commented 1 year ago

but someone with write access

sorry, just deprived you of that excuse