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

Add "Get Projects" and "Find By Id" from Production Service #251

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SuperJMN
Copy link
Contributor

Implement service to get projects from the real service and to find a given project by ID.

Includes the refactors and improvements that get this done.

… given project by ID.

Includes the refactors and improvements that get this done

public static class DependencyFactory
{
public static IRelayService GetRelayService(ILoggerFactory loggerFactory)
Copy link
Member

Choose a reason for hiding this comment

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

just wondering why not use the standard dependency injection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I chose this approach because the number of dependencies is still relatively small and adding a DI container felt like overengineering at this stage. However, if you think we should switch to standard DI, I can prepare a PR with the changes.

Copy link
Member

Choose a reason for hiding this comment

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

yes definitely we will use DI look at the client project and how we utilize the DI there we should do the same

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what DI you had in mind to use but the default on the web code is Microsoft.Extensions.DependencyInjection.ServiceProvider

image

So probably we just use the same one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use use of Microsoft.Extensions.DependencyInjection. It's done and it works perfectly, but I think that change should be done in a follow-up PR. It's a bit of scope and I'd like to keep it separated. What do you think?

namespace Angor.Model.Implementation;

// TODO: This is copied from Angor.Client
public class NetworkConfiguration : INetworkConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

hmm I dont like this, but it is temporary code then I guess it is ok for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I decided to copy it

public class Stage : IStage
{
public DateOnly ReleaseDate { get; set; }
public uint Amount { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

money we represent as long normally

Copy link
Member

Choose a reason for hiding this comment

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

unless this is the stage representation so it is a percent in which case it should be a decimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the stage representation and it's a number in sats internally, that can be converted to whatever we need in the UI (BTC or sats)

);

lookupResultsHelper = Lookup
.ObserveOn(RxApp.MainThreadScheduler)
Copy link
Member

Choose a reason for hiding this comment

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

why do you need to use a schedular MainThreadScheduler? is it because there are calls over the wire?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ViewModels needs to ensure properties are modified in the UI thread, because they are bound to the UI. Avalonia (and other GUI technologies) requires this.

But those calls to ObserveOn are not needed in this case because ReactiveUI handles this internally.

@@ -0,0 +1,20 @@
using AngorApp.Model;

namespace Angor.Model.Implementation.Projects;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This model belongs to the UI only right? perhaps the namespace should reflect that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. I've modified the namespaced to reflect that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants