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

Enhancement/minor documentation update #141

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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.

None yet

2 participants