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

Source generator - Reference named styles #44

Closed
sveinungf opened this issue Apr 6, 2024 · 6 comments
Closed

Source generator - Reference named styles #44

sveinungf opened this issue Apr 6, 2024 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@sveinungf
Copy link
Owner

sveinungf commented Apr 6, 2024

Background and motivation

Right now there is no way to use any styling with the source generator. Enabling styling by only using attributes (for example as suggested in #31 and #43) is an approach that would be easy to use, but requires a lot of work (and potentially many attributes) before all parts of styling would be supported.

If there would be a way to first add named styles to a spreadsheet, then these could be referenced with attributes on properties. This would be a way to have all supported forms of styling with the source generator.

API proposal

namespace SpreadCheetah;

public sealed class Spreadsheet
{
    public StyleId AddStyle(Style style, string name);
}
namespace SpreadCheetah.SourceGeneration;

[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
public sealed class CellStyleAttribute(string styleName) : Attribute;

API usage

public class Person
{
    [CellStyle("My FirstName style")]
    public string FirstName { get; set; }
}

[WorksheetRow(typeof(Person))]
public partial class PersonRowContext : WorksheetRowContext;
await using var spreadsheet = await Spreadsheet.CreateNewAsync(stream);
await spreadsheet.StartWorksheetAsync("Sheet 1");

var style = new Style();
style.Font.Bold = true;

spreadsheet.AddStyle(style, "My FirstName style");

var person = new Person { FirstName = "Ola" };
await spreadsheet.AddAsRowAsync(person, PersonRowContext.Default.Person);
@sveinungf
Copy link
Owner Author

This has been implemented in version 1.17.0.

@cajuncoding
Copy link

cajuncoding commented Sep 28, 2024

It would be nicer if the style names weren't forced to be String values -- which encourages use of magic strings by devs. But, a custom Enum would be really nice.... you could implement this with a Generic overload that simply uses the .ToString() of whatever is passed in as the Key to method and under the hood continue to use the string based key correllation, but allow Dev teams to create an Enum of styles that makes sense for a report.

@sveinungf
Copy link
Owner Author

I see your point. However since the named styles also can be shown in the Excel UI, I want to be able to use any character in the name. If the name was based on e.g. an Enum, then having a white-space in the name would be problematic.

If you want to use an Enum instead, perhaps you can do it by adding your own extension method? E.g.:

public static StyleId AddStyle<T>(this Spreadsheet spreadsheet, Style style, T name)
{
    return spreadsheet.AddStyle(style, name.ToString());
}

For the CellStyle attribute you can reference an Enum with nameof:

public class MyClass
{
    [CellStyle(nameof(MyEnum.Bold))]
    public int MyValue { get; set; }
}

@cajuncoding
Copy link

cajuncoding commented Oct 3, 2024

Yeah. that works, but having to use nameof() isn't ideal.

I was thinking about it some more and realized that the main issue is that your Attribute classes are sealed so we cannot override to augment with our own custom functionality. For example, this same issue exists with Microsoft's own AuthorizeAttribute which takes in Role names as strings (mainly because parameters to attributes have to be valid constant expressions), but by overriding it we can provide our own elegant constructors that take in our Role Enum and the code is nice and tidy; internally we map the Enum to a string value (e.g. role.ToString()).

So what are your thoughts on un-sealing the attributes to make it more flexible for consumers?

[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
public MyAppCellStyleAttribute : CellStyleAttribute
{
    public MyAppCellStyleAttribute(CellStyleEnum styleEnum) : base(styleEnum.ToString())
    { }
}

@sveinungf
Copy link
Owner Author

Unfortunately I don't think it would be trivial to make that work. A source generator only "looks" at the code during compilation, it doesn't execute the code. In your example the source generator could see there is a call to ToString(), but it won't actually call the ToString() method. Maybe it is obvious what ToString() should return in this case, but there wouldn't be anything to prevent you from writing more complex C# code in the attribute. We could then easily end up in a situation where it will be very hard (or maybe impossible) for the source generator to figure out what the resulting string should be.

I would guess deriving from AuthorizeAttribute works because whatever reads that attribute is using reflection and not a source generator. In that case it all happens during runtime, so the runtime can then also execute your custom code.

@cajuncoding
Copy link

Ahhh, ok yes we haven't embraced using source generators yet so I often don't think about the implications there.

In our current feature I'm working on, we've unfortunately had to stop using the source generation approach in some cases with SpreadCheetah due to the need for more flexible cell styling and dynamic changes for each row.... so I wasn't thinking about source generation. Thanks for the feedback 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants