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

Dev/src gen cell value mapper #69

Merged
merged 36 commits into from
Sep 4, 2024
Merged

Dev/src gen cell value mapper #69

merged 36 commits into from
Sep 4, 2024

Conversation

Ashymonth
Copy link

Preview version of source gen with CellValueMapper

@Ashymonth Ashymonth changed the base branch from main to dev/src-gen-cellstyle August 23, 2024 18:16
@Ashymonth
Copy link
Author

Ashymonth commented Aug 23, 2024

This is how I think to implement source gen with CellValueMapper. I'd like to discuss a few questions before I start to implement full functionality.

  1. Should I take care of CellValueTruncateAttribute? I think not, but maybe I missed something. Should I raise a warning message if the user applies CellValueTruncateAttribute and CellValueMapperAttribute?
  2. I made an array of ICellValueMapper static. Is that correct?
  3. I'd like to add the ability to use CellValueMapperAttribute on a class and property. In my case, I have 50+ properties on a class and 1 behavior to handle null values in properties. And if the attribute exists on the class and on the property, the type provided in the property will be used. Do you agree with this feature?
  4. Maybe you have another look at this feature? @sveinungf

@sveinungf
Copy link
Owner

Thanks for the PR!

I made a quick suggestion how to support this in #67, but after some more thought I'd like to make some changes to my initial suggestion:

  • Instead of CellValueMapper, I think CellValueConverter is a better name. Both EF Core and System.Text.Json use the term "converter" for similar functionality.
  • I think we should use an abstract class instead of an interface. It will then be possible to add more functionality to CellValueConverters later without introducing a breaking change. The dotnet team also uses abstract classes for similar cases: Why dotnet team prefers abstract classes over interfaces? dotnet/runtime#84120
  • I would like CellValueConverters to only handle cell values, and not styling, formulas, etc. It will then be more modular and can be used together with other source generator attributes that will be added in the future. This basically means the abstract method on CellValueConverter should have return type DataCell.
  • We need to add a constructor to StyledCell with signature StyledCell(DataCell dataCell, StyleId? styleId) so that the source generator can create StyledCell when using a CellValueConverter.

Here is how that would look in code:

public class MyObject
{
    [CellValueConverter(typeof(NullToDashConverter))]
    public int? Number { get; set; }
}

public class NullToDashConverter : CellValueConverter<int?>
{
    public DataCell ConvertToCell(int? value)
    {
        return value is null ? new DataCell("-") : new DataCell(value.Value);
    }
}

public abstract class CellValueConverter<T>
{
    public abstract DataCell ConvertToCell(T value);
}

Support for selecting a style based on the cell value probably doesn't belong in this PR, but to handle that I am considering a similar approach. That feature would depend on getting a style by its name. In code it could look like this:

public class MyObject
{
    // TODO: Better name for the attribute?
    [CellStyleProvider(typeof(NullableIntStyleProvider))]
    public int? Number { get; set; }
}

public class NullableIntStyleProvider : CellStyleProvider<int?>
{
    public string? GetStyleName(int? value)
    {
        return value is null ? null : "My style";
    }
}

public abstract class CellStyleProvider<T>
{
    public abstract string? GetStyleName(T value);
}

Now, to answer your questions:

  1. CellValueConverter will be incompatible with CellValueTruncate, so we should emit an error if attempting to use both attributes on a property.
  2. I think it can be a static array. We should avoid duplicates in that array, so if two properties use the same CellValueConverter, it should only lead to a single instance in the array. Note that it is also possible to use multiple WorksheetRow attributes on a single context class, which will lead to multiple RowTypes in the source generator. For that case we will need to avoid duplicate static arrays in the generated code.
  3. It makes sense to be able to set a CellValueConverter on a higher level, either on the class or perhaps even on the whole worksheet/spreadsheet. Using the same attribute on the class level as is being used on properties could work, I'm just a bit unsure about the readability, if it becomes unclear what properties the converter applies to. I'm not sure what the best approach here would be.

@Ashymonth Ashymonth changed the base branch from dev/src-gen-cellstyle to main September 1, 2024 10:51
@Ashymonth Ashymonth changed the base branch from main to dev/src-gen-cellstyle September 1, 2024 11:01
@Ashymonth Ashymonth marked this pull request as ready for review September 2, 2024 07:22
@Ashymonth Ashymonth changed the base branch from dev/src-gen-cellstyle to main September 2, 2024 07:36
@Ashymonth
Copy link
Author

I decided not to add support for the class level attribute for this time, because it requires another attribute with a different name and the ability to add it multiple times for different types. The current CellValueConverterAttribute cannot be applied multiple times to a property. I also added support for combining CellValueConverterAttribute and CellStyleAttribute.
I also added new analyzer errors, but I didn't include them in the AnalyzerReleases.Shipped.md file because I don't know the release policy.

@sveinungf
Copy link
Owner

Thanks! 👍 I'll have a look.
Regarding having an attribute on class level, that can be added later. I would like to have some kind of common approach of handling defaults, but I haven't figured out how that should look like yet. The new analyzer errors I will add to that file when I release the next version.

@sveinungf sveinungf changed the base branch from main to dev/src-gen-cellvalueconverter September 4, 2024 19:58
@sveinungf
Copy link
Owner

This looks very good, thank you! I will make some minor adjustments and once that is ready I will release a new version on NuGet. I hope to have it ready during the weekend.

@sveinungf sveinungf merged commit 0c29c21 into sveinungf:dev/src-gen-cellvalueconverter Sep 4, 2024
1 check passed
@Ashymonth Ashymonth deleted the dev/src-gen-cell_value_mapper branch September 4, 2024 20:11
@Ashymonth
Copy link
Author

okay
thank you for the help and awesome library)

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