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

Implementation of Try-XYZ pattern #277

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

Implementation of Try-XYZ pattern #277

wants to merge 9 commits into from

Conversation

timyhac
Copy link
Collaborator

@timyhac timyhac commented Oct 2, 2022

No description provided.

@timyhac
Copy link
Collaborator Author

timyhac commented Oct 2, 2022

I'm not super happy with this, particularly with the TryRead and TryReadAsync methods.
If it was possible, I would have preferred the method signatures to have out parameters for value, and the return value as either a bool for synchronous methods, or a Task<bool> for asynchronous, to indicate success.

e.g.

bool TryRead(out object value); 
Task<bool> TryReadAsync(out object value);

The sync version is valid C#, but the async version is not. You can't have out parameters on async methods.

Another option would be to not return the value at all, but then the API would be different to the Read(...) / ReadAsync(...) methods.

We could convert the Read(...) / ReadAsync(...) methods to return void and Task for symmetry, but then we would lose that valuable feature discussed in #93

@timyhac
Copy link
Collaborator Author

timyhac commented Oct 2, 2022

Thinking about it now (after a long walk), not returning the values is probably less jarring for an API consumer than returning a tuple.

@jkoplo
Copy link
Member

jkoplo commented Oct 2, 2022

I'm still skeptical of the 'try' pattern. That said I've also never quite liked that Read doesn't return a value... We discussed that at some point.

Maybe we should do a call and go through the API holistically? Design an ideal state and aim for a 2.0 version?

…ask<bool> instead of the tuple with that and the Value.
@timyhac
Copy link
Collaborator Author

timyhac commented Oct 2, 2022

I'm still skeptical of the 'try' pattern. That said I've also never quite liked that Read doesn't return a value... We discussed that at some point.

Maybe we should do a call and go through the API holistically? Design an ideal state and aim for a 2.0 version?

That sounds good @jkoplo

@kyle-github
Copy link
Member

Let me know if there are changes that would be needed in the core library. I think it may be about time to rethink the core API as well though I have not had any flashes of insight into how it might be better. All the other libraries expose the PLC as the base object and some do not have tag objects at all, they just use strings or register numbers.

@timyhac
Copy link
Collaborator Author

timyhac commented Oct 3, 2022

Let me know if there are changes that would be needed in the core library. I think it may be about time to rethink the core API as well though I have not had any flashes of insight into how it might be better. All the other libraries expose the PLC as the base object and some do not have tag objects at all, they just use strings or register numbers.

@kyle-github not about the core library per se, but we do get a lot of repeat questions. What do you think about something like an FAQ?

@kyle-github
Copy link
Member

That sounds like a good idea. Can you edit the wiki on the main project? If not, I can start a FAQ page.

One challenge here is that it seems like most of the users are using the C# wrapper. I don't get that many using the core C library anymore.

@jkoplo
Copy link
Member

jkoplo commented Oct 4, 2022

FAQ: There are definitely some things that belong in the main repo docs/faq and some things that are probably c# specific. How cheesy would it be for the c# wiki to link to the main wiki on certain topics? Also, I'm using DocFX in my real job for a few things. If I can carve out some time, I'd love to get that spun up on libplctag.net and maybe host it in a github.io site.

I just checked the nuget stats and the native wrapper has almost 40k downloads. That's pretty legit.

@kyle-github
Copy link
Member

I don't have a problem with FAQs referencing other FAQs.

40k downloads is great! I am not sure I can get statistics for downloads of the core library out of GitHub without paying for a premium account.

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