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

Separate non-Roman verse setting to restore USX compliance in VerseRef #999

Merged
merged 8 commits into from
Dec 2, 2020

Conversation

SamDelaney
Copy link
Contributor

@SamDelaney SamDelaney commented Nov 24, 2020

Done in response to issues with PR #997


This change is Reviewable

Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

Since this adds a new method the commit message should have a line:

+semver:minor

Also please add a line to CHANGELOG.md with the new method.

SIL.Scripture/VerseRef.cs Outdated Show resolved Hide resolved
SIL.Scripture/VerseRef.cs Show resolved Hide resolved
SIL.Scripture.Tests/VerseRefTests.cs Show resolved Hide resolved
SIL.Scripture/VerseRef.cs Show resolved Hide resolved
Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

Thanks for adding the ignored tests!

I noticed a few more things...

SIL.Scripture/VerseRef.cs Outdated Show resolved Hide resolved
SIL.Scripture/VerseRef.cs Outdated Show resolved Hide resolved
SIL.Scripture/VerseRef.cs Show resolved Hide resolved
SIL.Scripture/VerseRef.cs Show resolved Hide resolved
SIL.Scripture.Tests/VerseRefTests.cs Outdated Show resolved Hide resolved
Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

:lgtm: now 😄

Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SamDelaney)


SIL.Scripture/VerseRef.cs, line 282 at r3 (raw file):

		/// </summary>
		/// <returns><c>true</c> if the verse was set successfully </returns>
		bool TrySetVerse(string value, bool romanOnly = true)

nit: for a new method I think it would be clearer not to use a default value, especially since it's used only twice. But that might be a matter of personal preference...

Copy link
Contributor Author

@SamDelaney SamDelaney left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ermshiperete)


SIL.Scripture/VerseRef.cs, line 282 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

nit: for a new method I think it would be clearer not to use a default value, especially since it's used only twice. But that might be a matter of personal preference...

My thought process was that the default value helps indicate the preference of the USX standard for Latin numerals. I removed the default value and instead left a comment to that effect.

Copy link
Member

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@FoolRunning
Copy link
Collaborator

@ermshiperete, It looks like this is waiting for checks to pass, but they look like they passed in TC. Are you able to override that and merge this PR? Could you then do a merge from master to the NuGet branch?

@ermshiperete
Copy link
Member

Retriggering the builds on TC did the trick. Unfortunately I won't be able to do a merge from master into feature/nuget until next week.

@ermshiperete ermshiperete merged commit 2cab71e into sillsdev:feature/nuget Dec 2, 2020
@FoolRunning
Copy link
Collaborator

@ermshiperete, The merge from master to feature/nuget is now holding up the Paratext 9.1 release (we're waiting for the changes in #998). Are you deliberately holding off on doing the merge for some reason or is it just having the time to do it? If it's just time, I can find someone else that can do it.

@ermshiperete
Copy link
Member

@FoolRunning I just don't have time this week, so if somebody else can do it - great!

@FoolRunning
Copy link
Collaborator

@ermshiperete, Tom Bogle created a PR and we did it that way. Thanks for making it so easy to create the NuGet packages. :)

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