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

Pull Request welcome? InsertRows, SearchCellsByValue, ReplaceCellValue #71

Open
BombusSoftware opened this issue Dec 29, 2024 · 5 comments
Labels
completed A bug was fixed or question answered and the issue will be closed soon enhancement New feature or request

Comments

@BombusSoftware
Copy link
Contributor

In my small project, I use nanoXLSX to create a simple Excel file.
I open an existing file (template) and replace placeholders with the desired values.
For multiple records, I add new rows to the Excel sheet.

To achieve this, I have extended nanoXLSX with the methods listed below.
I am unsure if these methods would be of interest for nanoXLSX.
Should I make them available to you and create a pull request?
(This would be my very first pull request ever!)

These are the methods:

public void InsertRow(int rowNumber, int numberOfNewRows)
Adds a specified number of new rows below the given row.
The format of the upper row is applied to the new rows.
Note: Formulas are not adjusted!

public Cell FirstCellByValue(object searchValue)
Searches for the first cell containing the specified value and returns the cell.

public Cell FirstOrDefaultCell(Func<Cell, bool> predicate)
Searches for the first cell that matches the given predicate and returns the cell.

public int ReplaceCellValue(object oldValue, object newValue)
Finds all cells with the value "oldValue" and replaces it with "newValue."

I have already created a fork and a new branch in it. However, I have not yet pushed the commit...

@BombusSoftware BombusSoftware added the enhancement New feature or request label Dec 29, 2024
@rabanti-github
Copy link
Owner

rabanti-github commented Dec 30, 2024

Hi,
Thank you for your interest in NanoXLSX and the proposed additions.
PRs are always very welcome.
I have some questions about the mentioned functions:

  • InsertRow: What are you inserting there? Just the row definitions, or are you defining empty cells, based on the existing columns?
  • FirstOrDefaultCell: I assume it returns null (is Default?), if the value type is not supported or no value like that fund, right?
  • FirstOrDefaultCell(object searchValue) and FirstOrDefaultCell(Func<Cell, bool> predicate): Have you considered a search direction, like the enum CellDirection?
  • ReplaceCellValue: I assume, the int result is the number of replacements, right? Are formulas (or Cell objects in general) also covered by this?

Over all, the functions sounds interesting. If you want, you can create a PR with the target branch DevPR. I would review the PR as soon as possible.

  • Unit test of the functions are highly welcomed and encouraged, but could be added also later
  • I might add or adapt code documentation before merging to master, if missing for some functions
  • Depending on the extent of the PR, there may be some additional question or change requests. Most important is the new features add a consistent and non-API-braking value to the library.
  • Don't worry about review comments. Code Reviews are a big part of my daily job, and my peers have not yet complained about my review style :-)

I'm looking forward to your answer and your possible PR.
Thank you in advance

@BombusSoftware
Copy link
Contributor Author

Thank you for your response!

Here my answers to your Questions:

InsertRow: What are you inserting there? Just the row definitions, or are you defining empty cells, based on the existing columns?

Exactly, I insert new, empty cells and adopt the format from the first row.

FirstOrDefaultCell: I assume it returns null (is Default?), if the value type is not supported or no value like that found, right?

That's correct. It returns null or the Cell.

FirstOrDefaultCell(object searchValue) and FirstOrDefaultCell(Func<Cell, bool> predicate): Have you considered a search direction, like the enum CellDirection?

No, that hasn’t been considered, but it’s an interesting idea.
It should be relatively easy to add this. (Theoretically ;) )

ReplaceCellValue: I assume, the int result is the number of replacements, right? Are formulas (or Cell objects in general) also covered by this?

I am not taking the CellType into account here.
I haven't dived deep enough into nanoXLSX yet, especially regarding formulas. While the DataType indicates "Formula", I don’t find the formula in .Value, only the calculation result...
Therefore, DataType is ignored (so far).


Additional Explanation:

As far as I understand, nanoXLSX is designed to build an Excel file cell by cell.
But I'm way too lazy for that. ;) I like working with templates and therefore create a finished Excel file with placeholders.
For example:

Date Amount Balance
Payments \$Payments\$
Taxes \$Taxes\$
Other Stuff

For Payments, there is only one row. However, there could be multiple rows, so I need the InsertRow method.
The same applies to Taxes.
The result could look something like this:

Date Amount Balance
Payments 31.12.2024 5.25
15.01.2025 10.00
18.01.2024 -2.23
Taxes 31.12.2024 5.00
30.03.2024 5.00
Other Stuff

I add 2 rows (for Payments) and 1 row (for Taxes) and populate the new cells with the values.
I take a somewhat "brutal" approach: I remove all cells below \$Payments\$, add new cells, and populate them with values. Then, I re-add the previously removed cells (but with a new address and new key).
For large Excel files, this might be somewhat inefficient. The problem is that the key in a Dictionary cannot be modified.


Then, I’ll take the plunge and make a pull request – it will be my first! :)
I’ve added tests, but I often lack creativity in this area.

@rabanti-github
Copy link
Owner

rabanti-github commented Jan 1, 2025

Thanks for the detailed explanation and the PR. I will look at it as soon as possible in January.

An additional note about the purpose of NanoXLSX/PicoXLSX/NanoXLSX4j:
The libraries were initially created with the goal to create Excel files without the struggle of Office Interop or knowledge of OOXML. Of course, there is still a portion of OOXML, especially in style definitions. But in the end, NanoXLSX/PicoXLSX are intended to be driven by business data (as you would use Excel), and not by some obscure programmatical definitions (I'm looking at you POI ;-P). So, I think, your template approach can contribute a good value to NanoXLSX/PicoXLSX/NanoXLSX4j, although I have not looked into the PR code yet.

Furthermore, I wish you a happy new year

@rabanti-github
Copy link
Owner

A new version 2.6.0 of NanoXLSX (and PicoXLSX 3.4.0) was released some minutes ago and is already online as NuGet package.
I fixed one additional null-pointer issue in one of the new functions, added/adapted some test cases, and added the function InsertColumn, according to InsertRow.
Thanks again for the contribution.
Please let me know if something is not working as expected.

Besten Dank für die Features 👍

@rabanti-github rabanti-github added the completed A bug was fixed or question answered and the issue will be closed soon label Jan 12, 2025
@BombusSoftware
Copy link
Contributor Author

Thanks for the information!
I'm very pleased that I was able to make a small contribution to nanoXLSX! 😊

The new package works wonderfully and does what it should.
InsertColumn is a very good idea, I might be able to use that in the future.

Thanks for letting me learn so much about GitHub and unit testing!
If I will have new ideas for nanoXLSX again, I'll get back!

Viele Grüße aus Bayern in die Schweiz. 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed A bug was fixed or question answered and the issue will be closed soon enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants