-
Notifications
You must be signed in to change notification settings - Fork 2
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 support for creating data providers in C# and apply a few Resharper suggestions #241
Conversation
This doesn't add USFM support yet. It just adds a simple C# data provider to start. Once ParatextData integration is working, this code can be used to create a USFM data provider. |
33278c7
to
ac2082d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have enough C# to say this looks ok to me but not enough to know it's ok. However given it works that's probably enough to merge it.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lyonsil)
c-sharp/MessageTransports/PapiClient.cs
line 82 at r1 (raw file):
// Do not change this code. Put cleanup code in 'Dispose(bool isDisposing)' method Dispose(isDisposing: true); // ReSharper disable once GCSuppressFinalizeForTypeWithoutDestructor
BTW is ReSharper doing some kind of linting for you? If so how would someone else run/check it?
c-sharp/NetworkObjects/DataProvider.cs
line 33 at r1 (raw file):
: base(papiClient) { // "-data" is the prefix used by PAPI for data provider names
BTW you wrote 'prefix' but you add it as a 'suffix', did I miss something or is this a typo?
Code quote:
prefix
c-sharp/NetworkObjects/TimeDataProvider.cs
line 30 at r1 (raw file):
protected override ResponseToRequest HandleRequest(string functionName, string[] arguments) { return functionName switch
Oh, switch expressions are cool! I had to look them up though.
c-sharp/Properties/launchSettings.json
line 11 at r1 (raw file):
} } }
BTW the two red symbols at the end here indicate there should be a blank line at the end of the file but there isn't.
extensions/lib/hello-world/hello-world.web-view.tsx
line 58 at r1 (raw file):
}; const [currentTime] = useData.Time<TimeDataType, 'TimeData'>(
BTW I could be wrong but I think that the name after useData.
and the string literal in the type should match, however I think it's not currently checking for this. I.e. in your case Time
and 'TimeData'
don't match when they should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)
c-sharp/MessageTransports/PapiClient.cs
line 82 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW is ReSharper doing some kind of linting for you? If so how would someone else run/check it?
I finally got a Resharper license, and it started popping things up in Visual Studio when I opened the project. So I guess the answer is "open the C# project in Visual Studio with Resharper installed." That doesn't align with how some of our other tools have been working, but it's the only answer I know.
c-sharp/NetworkObjects/DataProvider.cs
line 33 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW you wrote 'prefix' but you add it as a 'suffix', did I miss something or is this a typo?
Indeed that was a mistake. Fixed.
c-sharp/NetworkObjects/TimeDataProvider.cs
line 30 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
Oh, switch expressions are cool! I had to look them up though.
Actually they were new to me, too. I learned about them because Resharper suggested I use one here instead of a traditional switch statement. This form is a little more compact, so I kept it.
c-sharp/Properties/launchSettings.json
line 11 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW the two red symbols at the end here indicate there should be a blank line at the end of the file but there isn't.
Hmm. It's a little funny that the file was automatically created by one tool (Visual Studio) to use for itself while another tool (Reviewable) says the file is wrong. I think this is just tools disagreeing with each other. What is "right" is relative here, I think.
extensions/lib/hello-world/hello-world.web-view.tsx
line 58 at r1 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW I could be wrong but I think that the name after
useData.
and the string literal in the type should match, however I think it's not currently checking for this. I.e. in your caseTime
and'TimeData'
don't match when they should.
I thought it was looking for the name of the property in the type. The TS code backing this is a little dense, and it wasn't obvious to me.
I can say for certain that Time
after useData
causes the framework to call getTime
on the network object. That one is super important. Everything inside the angle brackets seems to be all about type checking in TS. That's why I thought it was focused on that. I could just call both "Time" and make it ambiguous, but this might be something good to clarify with @tjcouch-sil when he's back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)
c-sharp/Properties/launchSettings.json
line 11 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Hmm. It's a little funny that the file was automatically created by one tool (Visual Studio) to use for itself while another tool (Reviewable) says the file is wrong. I think this is just tools disagreeing with each other. What is "right" is relative here, I think.
I actually think this file should be deleted. I vaguely recall having this same file be created on the initial C# code implementation and TJ requesting it get removed since it's not needed.
739af29
to
1e74998
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)
c-sharp/Properties/launchSettings.json
line 11 at r1 (raw file):
Previously, FoolRunning (Tim Steenwyk) wrote…
I actually think this file should be deleted. I vaguely recall having this same file be created on the initial C# code implementation and TJ requesting it get removed since it's not needed.
Ok. I removed it and added it to the .gitignore
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
c-sharp/Properties/launchSettings.json
line 11 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Ok. I removed it and added it to the
.gitignore
file.
Actually I don't know what this file is. The VSCode file I mentioned not generating is .vscode/tasks.json
or something along those lines if I remember correctly. That file adds "tasks" to VSCode that we already have because VSCode scrapes the package.json
file and adds some of the scripts as tasks.
I'm not sure you want to remove this file.
This change is