adyeths / u2o

USFM to OSIS bible format converter.
The Unlicense
19 stars 6 forks source link

periph element attributetext when there is none? #84

Closed DavidHaslam closed 5 years ago

DavidHaslam commented 5 years ago

These lines (1473-1477) of code:

                if attributetext is not None:
                    attributetext = '{}{}{}'.format(
                        '<!-- USFM Attributes - ',
                        attributetext,
                        ' -->')

produce this when there is no attribute text:

<!-- USFM Attributes -  -->

Although this is merely an XML comment, I still think that logically, the hyphen and 2 spaces should not be included when there's no attributetext. It should produce simply:

<!-- USFM Attributes -->
adyeths commented 5 years ago

What is the usfm that resulted in this?

DavidHaslam commented 5 years ago

Henry T. Anderson's 1864 "Civil War" New Testament https://github.com/BeTheLight/ENG-B1-Anderson1864-pd-USFM

It’s what I’m currently engaged with.

DavidHaslam commented 5 years ago

It’s obviously trivial inasmuch as it’s in the remark.

I spotted it almost in passing while I was locating spurious instances of -

adyeths commented 5 years ago

which file specifically produced this? Because there shouldn't be any empty attributetext comments like this.

DavidHaslam commented 5 years ago

One of the files has 00 in the file name.

It’s either that or the next one with 40 in the filename.

Apart from the last file with 69 all the others have the USFM standard NT numbering 41 through 67.

Wasn’t this almost intuitive?

adyeths commented 5 years ago

I don't have any context with regards to where the problem with the attributetext occurred. That's what I'm trying to find. Without that context I can't determine why it happened when the code in question was written specifically to avoid putting random empty comments in the osis output.

If I knew where in the usfm files it happened, I could look at it and see why and possibly fix it.

DavidHaslam commented 5 years ago

Only two instances of \periph as far as I can recall.

These are in one of those small files at front or back of the NT.

My PC is in sleep mode at this time of night, so I’m replying from memory.

DavidHaslam commented 5 years ago

I have given the context.

I responded last night from memory as soon as you asked.

Please re-open the issue.

I can add more details once I’m at my workstation.

DavidHaslam commented 5 years ago

The fact that the issue title starts with the word periph is also to give the context.

There are no instance of \periph in files for MAT to REV.

adyeths commented 5 years ago

It should be fixed. Thats why I closed it.

On Wed, Feb 13, 2019, 12:46 AM David Frank Haslam <notifications@github.com wrote:

I have given the context.

I responded last night from memory as soon as you asked.

Please re-open the issue.

I can add more details once I’m at my workstation.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/adyeths/u2o/issues/84#issuecomment-463068070, or mute the thread https://github.com/notifications/unsubscribe-auth/ANRjb4JAwPRANpCUBIkjtij_CSU0mpzDks5vM6atgaJpZM4a4GMC .

DavidHaslam commented 5 years ago

Ah - sorry.

I just saw “closed” after the notified previous comment.

Aside: I’m writing from my iPhone using the CodeHub app.

I hadn’t yet spotted that you’d committed a change.

DavidHaslam commented 5 years ago

Is there a related push ?

Or is it still in your pipeline?

adyeths commented 5 years ago

last commit should fix this.

On Wed, Feb 13, 2019, 12:56 AM David Frank Haslam <notifications@github.com wrote:

Is there a related push ?

Or is it still in your pipeline?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/adyeths/u2o/issues/84#issuecomment-463070063, or mute the thread https://github.com/notifications/unsubscribe-auth/ANRjb_8KXLZGS0BSd5qbH58peKZFOhJiks5vM6kqgaJpZM4a4GMC .

DavidHaslam commented 5 years ago

Fix tested and confirmed. Many thanks.