Skip to content

Conversation

@BeyondMySouls
Copy link

Improve documentation for readme, and xml documentation for Action/Func fields parameters.

@codecov
Copy link

codecov bot commented Aug 29, 2021

Codecov Report

Merging #141 (58fea2e) into master (ff7f3b2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #141   +/-   ##
=======================================
  Coverage   93.80%   93.80%           
=======================================
  Files           9        9           
  Lines        1195     1195           
  Branches      171      171           
=======================================
  Hits         1121     1121           
  Misses         47       47           
  Partials       27       27           
Impacted Files Coverage Δ
ExcelMapper/ColumnInfo.cs 87.93% <ø> (ø)
ExcelMapper/ExcelMapper.cs 94.77% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff7f3b2...58fea2e. Read the comment docs.

@mganss
Copy link
Owner

mganss commented Aug 29, 2021

Thanks. TBH, I'm not a fan of the pseudo-syntax documentation for Func parameters. Is this used elsewhere?
I'd prefer just regular text comments like it's used e.g. for the predicate parameter of Enumerable.Where. Would you be willing to change the documentation comments in this way?

@BeyondMySouls
Copy link
Author

BeyondMySouls commented Aug 29, 2021

Sure no worries.

Thanks. TBH, I'm not a fan of the pseudo-syntax documentation for Func parameters. Is this used elsewhere?

public ColumnInfo SetPropertyUsing(Func<object, ICell, object> setProp)  
{ SetProp = (o, v, c) => setProp(v, c); return... }
public ColumnInfo SetPropertyUsing(Func<object, object, ICell, object> setProp) 
{ SetProp = setProp; return... }  
public ColumnInfo SetPropertyUsing<T>(Func<T, object, object> setProp) 
{ SetProp = (o, v, c) => setProp((T)o, v); return... }

While using several methods in my application, I had some issues to understand what you expect from these Func parameters. Therefore I had to dig in the source code to check where you pass those parameters and still, at a glance, do not understand what some of the parameters above are because they are named "(o, v, c)".

What I mean is that it is quite difficult to understand what you expect from each parameter in a Func, especially when you use primary types, the object type, or have several inputs. So I added these xml comments.

Just added some examples how it currently appears:
Screenshot (13)
Screenshot (12)

I'd prefer just regular text comments like it's used e.g. for the predicate parameter of Enumerable.Where. Would you be willing to change the documentation comments in this way?

Sorry, I do not understand what you mean here or where we should add those text comments. To be honest, I am quite busy but , if it is a quick addition, I don't mind.

@mganss
Copy link
Owner

mganss commented Aug 30, 2021

I agree with you about the need to better document the Func's parameters.

Sorry, I do not understand what you mean here or where we should add those text comments. To be honest, I am quite busy but , if it is a quick addition, I don't mind.

I mean instead of adding pseudo-code like Func<cellValue: object, cell: ICell, out property: object> after the original documentation sentence we should follow the example used in MSDN by adding a sentence that describes the parameters. For example:

Specifies a method to use when when setting the property value from the cell value. The first parameter of the function represents the value of the cell and the second parameter represents the cell itself. The function should return the value that will be assigned to the property.

@BeyondMySouls
Copy link
Author

Ok, I will add them.

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.

2 participants