Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forex, Futures, Crypto and Stock updates #51

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mccabe93
Copy link
Contributor

@mccabe93 mccabe93 commented Aug 10, 2022

  • Realtime quotes for crypto and stock.
  • FinancialScore in Advanced Data
  • Futures and Forex support for realtime and historical quotes
    Some cleanup and refactoring. Replaced doubles with decimals, fixed a typo (succes -> success)

- Realtime quotes for crypto and stock.
- Futures and Forex support for realtime and historical quotes
</PackageReleaseNotes>
<AssemblyName>FinancialModelingPrep</AssemblyName>
</PropertyGroup>
<PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert changes here

@@ -18,6 +19,19 @@ public StockTimeSeriesProvider(FinancialModelingPrepHttpClient client)
this.client = client ?? throw new ArgumentNullException(nameof(client));
}

public async Task<ApiResponse<List<StockQuoteResponse>>> GetQuoteAsync(string symbol)
{
const string url = "[version]/quote/[symbol]";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint already exists under api.CompanyValuation.GetQuoteAsync

{
public interface ICryptoMarketProvider
{
public Task<ApiResponse<List<CryptoItem>>> GetAvilableCryptocurrencies();
Task<ApiResponse<List<CryptoItem>>> GetAvilableCryptocurrenciesAsync();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task<ApiResponse<List<CryptoItem>>> GetAvailableCryptocurrenciesAsync();

}

public Task<ApiResponse<CryptoHistoricalPriceDailyItem>> GetDailyPrices(string symbol)
public async Task<ApiResponse<List<CryptoQuoteResponse>>> GetQuoteAsync(string symbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you just await the Task without doing anything with it you can remove the async/await keywords here.
Only the first caller in the call stack that needs to do something with the result needs to await it.


namespace MatthiWare.FinancialModelingPrep.Abstractions.Model
{
public interface ICurrentQuote
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to have this interface, just create the different models with the properties that they have.
Futures/Forex Quote models should not be forced to implement the Eps or PE properties

@Matthiee Matthiee added enhancement New feature or request Breaking Introduces breaking changes labels Aug 10, 2022
@Matthiee
Copy link
Member

Hi @mccabe93 The build is failing, also please check my previous review remarks, they have not been fixed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Introduces breaking changes enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants