Add DisplayName functionality #1
Conversation
|
Hi @NiSHoW, sorry for the delay! A lot of things needed to be done concurrently. This is a great addition, but I see it's mixed with dynamic filter attribute discovery (unrelated to the display name). Can we solve them one by one? |
|
Hey, I’m not sure I understand what’s wrong with my code. Could you please explain it to me a bit more? I’d love to help you out and send you a clean pull request. I just wrote some rough code for now. By the way, what do you think about serializing the jobdisplayname attribute? Should we serialize it as it is or should we get the displayname first and save the string instead? Oh, and I also deleted the code that adds the QueueAttribute attributes automatically. |
| var methodFiltes = job.Method.GetCustomAttributes(typeof(QueueAttribute), inherit: true).Cast<QueueAttribute>(); | ||
|
|
||
| //generate list of filters for dynamicjob | ||
| var dynamicfilters = new List<JobFilterAttribute>(filters?.ToArray() ?? Enumerable.Empty<JobFilterAttribute>()); |
There was a problem hiding this comment.
I think we should concentrate on description only in this pull request. Automatic filter attribute acquisition is great, but it has some gotchas – since arguments (or their types) of a target method can be unavailable at runtime, this solution will have a lot of limitations. Currently Hangfire.DynamicJobs extension requires all the filters to be passed explicitly, and this is the only obvious solution I see without hidden problems.
| //TODO: Check if is better save display name as resolved string (maybe when you use location resources can be cause crash?) | ||
| [CanBeNull] | ||
| [JsonProperty("dn", NullValueHandling = NullValueHandling.Ignore, ItemTypeNameHandling = TypeNameHandling.Auto)] | ||
| public JobDisplayNameAttribute DisplayName { get; } |
There was a problem hiding this comment.
I think we should expose the DisplayName property as a string one and calculate its value when creating a dynamic job. Such descriptions are often based on argument values (by using placeholders like {0} passed to the String.Format method), and since argument types can be unavailable at runtime, we shouldn't depend on them.
|
Kudos, SonarCloud Quality Gate passed!
|
I am sending you this pull request as a POC to add the functionality already present in hangfire to modify the DisplayName of the job in the dashboard using the appropriate attribute.
In the commits I added the DisplayName property to the DynamicJob object so that it is serialized and deserialized.
You will also find comments, on a possible alternative way (i.e. to resolve the display name at creation time)
Feel free to refuse the pull request and rewrite the code as you like, I thought it was more convenient to send you an example rather than a simple issue on the main hangfire repo.