EvotecIT / OfficeIMO

Fast and easy to use cross-platform .NET library that creates or modifies Microsoft Word (DocX) and later also Excel (XLSX) files without installing any software. Library is based on Open XML SDK
MIT License
286 stars 50 forks source link

Mislabeled Behavior? #211

Closed tmheath closed 7 months ago

tmheath commented 7 months ago
        /// <summary>
        /// Add paragraph after self but by allowing to specify section
        /// </summary>
        /// <param name="section"></param>
        /// <param name="paragraph"></param>
        /// <returns></returns>
        public WordParagraph AddParagraphAfterSelf(WordSection section, WordParagraph paragraph = null) {
            if (paragraph == null) {
                paragraph = new WordParagraph(section._document, true, false);
            }
            this._paragraph.InsertAfterSelf(paragraph._paragraph);
            return paragraph;
        }

I'm going through some stuff looking for things, if I end up changing anything then I'll add tests. I've been adding a lot of documentation that is either missing or otherwise badly worded. What is this method supposed to do when both section and paragraph is given? How I read it was basically whichever came last but that isn't what the code is doing.

If this needs updated then I can go ahead and do so this PR.

PrzemyslawKlys commented 7 months ago

This is used internally:

https://github.com/EvotecIT/OfficeIMO/blob/5f16f4f956fd32fb3d6f081a3a33761cc40af9c8/OfficeIMO.Word/WordSection.PublicMethods.cs#L16-L26

and

https://github.com/EvotecIT/OfficeIMO/blob/5f16f4f956fd32fb3d6f081a3a33761cc40af9c8/OfficeIMO.Word/WordSection.PublicMethods.cs#L28-L47

This could be hidden and made internal/private only. I don't think it doesn't make much sense outside of internal usage.

tmheath commented 7 months ago

Alright thanks, that answered this question.

Would you like me to change the scope of the method to internal in next pr or leave as is?

PrzemyslawKlys commented 7 months ago

Can be PR

tmheath commented 7 months ago

I also should update the signature for AddParagraphBeforeSelf then?

Making the change locally, will open a PR in the future.

tmheath commented 7 months ago

I've made the commit locally, let me know if AddParagraphBeforeSelf shouldn't be made private.

PrzemyslawKlys commented 7 months ago

If it has section then yes. But otherwise no

tmheath commented 7 months ago

BeforeSelf does not have a section argument, but an overload for AfterSelf does. I only need to make the definition with section as an argument private and leave both other method's defined public?

Private: AddParagraphAfterSelf(WordSection section, WordParagraph paragraph = null); Public: AddParagraphAfterSelf(); public AddParagraphBeforeSelf();

Asking for clarification, right now I moved all three to the private methods file.

PrzemyslawKlys commented 7 months ago

AddParagraphAfterSelf is pretty useful AddParagraphBeforeSelf as well. Basically you can insert new paragraph before the one you're currently operating with. The ones with section isn't really useful i guess. But i could be wrong.