Ecosyste.ms: Timeline
Browse the timeline of events for every public repo on GitHub. Data updated hourly from GH Archive.
junhaoliao created a review on a pull request on y-scope/clp-ffi-js
tested with a clp-ffi-py generated IRv2 file with various log levels. all good except 1. logs can be printed before returning null in `auto StreamReader::generic_decode_range`: https://github.com/...
junhaoliao created a review on a pull request on y-scope/clp-ffi-js
test with a clp-ffi-py generated IRv2 file with various log levels. all good except 1. logs can be printed before returning null in `auto StreamReader::generic_decode_range`: https://github.com/y-...
junhaoliao created a review comment on a pull request on y-scope/clp-ffi-js
see https://github.com/y-scope/clp-ffi-js/pull/35/files#r1884668582 it be a good idea to add a log print here before returning
junhaoliao created a review comment on a pull request on y-scope/clp-ffi-js
this isn't in the log viewer code as well - Can we add a log print before returning so that we know the input argument check fails here rather than at the range check?
junhaoliao created a review comment on a pull request on y-scope/clp-ffi-js
In a way I feel the inline comments seem a bit bloated. Instead, as a new convention, can we add a prefix to hint the enum member is for describing properties of the enum? e.g., ``` enum class F...
junhaoliao created a review comment on a pull request on y-scope/clp-ffi-js
I agree with @davemarco 's (1.) argument about the challenges of adding new log levels. However, I also recognize that the `NONE` value is used solely for log parsing (in read paths, not write pa...
kirkrodrigues created a review comment on a pull request on y-scope/clp-ffi-js
Yeah, we need to wait until we completely drop support for IRv1.
junhaoliao created a review comment on a pull request on y-scope/clp-ffi-js
> the current log level parsing is that it's a heuristic until we migrate to IRv2 entirely @kirkrodrigues Would CLP FFI also support parsing log level for IRv1 stream in the new `Deserializer` i...
kirkrodrigues created a review on a pull request on y-scope/clp-ffi-js
For the PR title, how about: ``` feat: Add support for log-level filtering of structured IR streams. ```
coderabbitai[bot] created a review comment on a pull request on y-scope/clp-ffi-js
_:warning: Potential issue_ **Add bounds checking for node ID increment.** The current node ID increment could potentially overflow. Consider adding bounds checking. ```diff auto StructuredIrUn...
coderabbitai[bot] created a review comment on a pull request on y-scope/clp-ffi-js
_:warning: Potential issue_ **Improve exception safety and bounds checking.** The method should use find() instead of at() to avoid potential exceptions, and validate integer bounds more strictly...
coderabbitai[bot] created a review on a pull request on y-scope/clp-ffi-js
**Actionable comments posted: 2** <details> <summary>🧹 Outside diff range and nitpick comments (1)</summary><blockquote> <details> <summary>src/clp_ffi_js/ir/StructuredIrUnitHandler.cpp (1)</summ...
coderabbitai[bot] created a review on a pull request on y-scope/clp-ffi-js
**Actionable comments posted: 2** <details> <summary>🧹 Outside diff range and nitpick comments (1)</summary><blockquote> <details> <summary>src/clp_ffi_js/ir/StructuredIrUnitHandler.cpp (1)</summ...
coderabbitai[bot] created a review comment on a pull request on y-scope/clp-ffi-js
_:warning: Potential issue_ **Improve exception safety and bounds checking** The method should use find() instead of at() to avoid potential exceptions, and validate integer bounds more strictly....
coderabbitai[bot] created a review comment on a pull request on y-scope/clp-ffi-js
_:warning: Potential issue_ **Add bounds checking and improve exception safety** The method should validate the timestamp value and use find() instead of at(). ```diff - auto const& timestamp...
coderabbitai[bot] created a review comment on a pull request on y-scope/clp-ffi-js
_:hammer_and_wrench: Refactor suggestion_ **Return a more appropriate error code** Since UTC offset changes aren't handled, consider returning `IRErrorCode_Unimplemented` instead of `IRErrorCode_...
coderabbitai[bot] created a review on a pull request on y-scope/clp-ffi-js
**Actionable comments posted: 3** <details> <summary>🧹 Outside diff range and nitpick comments (2)</summary><blockquote> <details> <summary>src/clp_ffi_js/ir/StructuredIrUnitHandler.cpp (2)</summ...
coderabbitai[bot] created a review on a pull request on y-scope/clp-ffi-js
**Actionable comments posted: 3** <details> <summary>🧹 Outside diff range and nitpick comments (2)</summary><blockquote> <details> <summary>src/clp_ffi_js/ir/StructuredIrUnitHandler.cpp (2)</summ...
kirkrodrigues created a review comment on a pull request on y-scope/clp-ffi-js
Same comment for `timestamp_optional_value`.
kirkrodrigues created a review comment on a pull request on y-scope/clp-ffi-js
Although what you wrote is grammatically correct, we have an [internal guideline](https://www.notion.so/yscope/WIP-Coding-Guidelines-9a308b847a5343958ba3cb97a850be66?pvs=4#eb038f1b23cf4f20988590250...
kirkrodrigues created a review comment on a pull request on y-scope/clp-ffi-js
Nit: ```suggestion ```
kirkrodrigues created a review comment on a pull request on y-scope/clp-ffi-js
```suggestion auto const& log_level_int = log_level_value.get_immutable_view<clp::ffi::value_int_t>(); ```