Addressing comments from previous PR. #8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…More robust code. Refactored and properly passing tests.
Romazes
suggested changes
Jan 2, 2026
Romazes
left a comment
Romazes
left a comment
There was a problem hiding this comment.
- update to .net 10
- What about reconnection logic? Did you test when the internet disconnects/reconnects?
- You need create test cases of subscribe/unsubscribe when you create
DataBentoProviderinstance. (NOT classDataBentoRawLiveClientexplicitly)- Look at the screen below.
- You need add TestCases to SymbolMapper.
| public DateTime Timestamp => Time.UnixNanosecondTimeStampToDateTime(TimestampNanos); | ||
|
|
||
| [Name("open")] | ||
| public long RawOpen { get; set; } |
There was a problem hiding this comment.
Can we simplify these initializations of properties?
private decimal _open;
[Name("open")]
public decimal Open { get => _open; set => _open = value * PriceScaling.PriceScaleFactor; }
| public long RawClose { get; set; } | ||
|
|
||
| [Ignore] | ||
| public decimal Open => RawOpen == long.MaxValue ? 0m : RawOpen * PriceScaling.PriceScaleFactor; |
There was a problem hiding this comment.
- When is the condition true? Can you add an explanation and example of csv's raw data?
- If the condition doesn't work, please remove it.
- It looks so ambiguous, with public properties like
OpenvsRawOpen.
| [Name("ts_event")] | ||
| public long TimestampNanos { get; set; } | ||
|
|
||
| public DateTime Timestamp => Time.UnixNanosecondTimeStampToDateTime(TimestampNanos); |
There was a problem hiding this comment.
- Did it miss
[Ignore]attribute? - Please, rename to
TimestampUtc. - Please, add xml proof that exactly UTC as https://databento.com/docs/standards-and-conventions/common-fields-enums-types#time-zone?historical=python&live=python&reference=raw
| [Name("ts_event")] | ||
| public long TimestampNanos { get; set; } | ||
|
|
||
| public DateTime Timestamp => Time.UnixNanosecondTimeStampToDateTime(TimestampNanos); |
There was a problem hiding this comment.
- Did it miss
[Ignore]attribute? - Please, rename to
TimestampUtc. - Please, add xml proof that exactly UTC as https://databento.com/docs/standards-and-conventions/common-fields-enums-types#time-zone?historical=python&live=python&reference=raw
| [Name("ts_event")] | ||
| public long TimestampNanos { get; set; } | ||
|
|
||
| public DateTime Timestamp => Time.UnixNanosecondTimeStampToDateTime(TimestampNanos); |
There was a problem hiding this comment.
- Did it miss
[Ignore]attribute? - Please, rename to
TimestampUtc. - Please, add xml proof that exactly UTC as https://databento.com/docs/standards-and-conventions/common-fields-enums-types#time-zone?historical=python&live=python&reference=raw
| /// <param name="market">The market</param> | ||
| /// <param name="expirationDate">Expiration date of the security(if applicable)</param> | ||
| /// <returns>A new Lean Symbol instance</returns> | ||
| public Symbol GetLeanSymbol(string brokerageSymbol, SecurityType securityType, string market, |
| /// </summary> | ||
| /// <param name="databentoSymbol">The databento symbol</param> | ||
| /// <returns>The corresponding Lean symbol</returns> | ||
| public Symbol GetLeanSymbol(string databentoSymbol) |
| /// </summary> | ||
| /// <param name="brokerageSymbol">The brokerage symbol</param> | ||
| /// <returns>A new Lean Symbol instance</returns> | ||
| public Symbol GetLeanSymbolForFuture(string brokerageSymbol) |
There was a problem hiding this comment.
Use GetLeanSymbol(...) instead of this. You can pass not all parameters to GetLeanSymbol
| } | ||
|
|
||
| // ignore futures spreads | ||
| if (brokerageSymbol.Contains("-")) |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Compile Include="..\models\*.cs" /> |
There was a problem hiding this comment.
Use Folder instead Compile
<ItemGroup>
<Folder Include="Models\" />
</ItemGroup>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addressed the comments made on PR 3 from Roman.
I use Task.Run() due to the message loop being blocking. If I don't use Task.Run() then the message flow to DataBento and the app flow is not correct.