DavidFeldhoff / al-codeactions

MIT License
17 stars 8 forks source link

Add parameters to Label Comment #138

Closed jwikman closed 2 years ago

jwikman commented 2 years ago

In #113 you and @DavidRoys added a Comment with %1, %2 etc when extracting a label - very useful!

But I would like to get the parameters added as default values for those placeholders.

Samples:

Message('Hello %1, %2', "No.", Name); Would then get the label newLabel: Label 'Hello %1, %2', Comment='%1="No."; %2=Name';

And Message('%1: %2', FieldCaption(Code), Code); Would be newLabel: Label '%1: %2', Comment='%1=FieldCaption(Code); %2=Code';

etc.

There are sure some border cases where it gets hard(er) to identify the parameters, but we can always just fallback to blank.

What do you think?

DavidRoys commented 2 years ago

Hi @jwikman. I'm in no position to give a definitive answer on this, but having looked at this problem for a while I can tell you the line of thinking that got us where we are. What you are suggesting is exactly what I wanted to do, but as you know the placement of arguments is different for Confirm compared to Error or Message (as the first argument for Confirm is a boolean for default). This means in order to do this properly we would need to identify the command that the string is within. This is possible (as there are other parts of David's add-on that do this) but parsing the code to find this is more work (for the extension not just the developer). One thing I didn't realise before David showed me something about how this all works is the solution for the code action is fully realised just in case you decide to invoke it. This would mean whenever you move the cursor to within a string that could potentially be a label and if the label has placeholders we would need to parse the object to determine the command within which the text is used and then also pull the additional arguments. This seemed like a lot of work, but again, totally worthwhile if it's doing something valuable. I then thought about the purpose of the comments. I think these are meant to be used for translators to help determine the context of the message. In that instance just subbing in the placeholder arguments doesn't really help. It might get you past the Codecop warning, but if we're not going to provide useful information, maybe we should just switch the warning off? David took the code that I wrote (which was pretty basic really) and made it really useful by including the snippet functionality (where your cursor is placed at the argument position and you can tab to the next) and I really like what David did with that. I guess what I'm saying in this comment is that I think it's a great idea but don't personally want to try to realise it. :-) Maybe David will have a crack at it though or maybe you fancy having a go?

jwikman commented 2 years ago

Hi @DavidRoys,

Just curious, what do you write in those comments if there are field captions and values in the placeholders?

I agree on that the purpose of the comments is to give the translators some kind of context to what they are translating, but I think that Label '%1: %2', Comment='%1=FieldCaption(Code); %2=Code'; is quite self explanatory.

There are, of course, cases where you would need to edit the suggested comment, but I believe that it would save some time - at least for me. :)

And we might start to support this for select scenarios

And if Confirm gets especially hard to implement, having the others would be great.

If none of you Davids' have the time to look into it, I might give it a go when the time allows. :-)

DavidFeldhoff commented 2 years ago

Wow, already a nice conversation here :) Thanks for sharing your thoughts to both of you! I haven't thought of the confirm, good hint @DavidRoys. I'm pretty sure I'll find some time the next days to look into it. If I won't make it this weekend, then I'll definitely look into it on Monday. If I encounter some scenarios we haven't considered yet, I might come back to you to ask what you'd expect :)

jwikman commented 2 years ago

That's super @DavidFeldhoff, looking forward to what you comes up with! 👍

DavidFeldhoff commented 2 years ago

I think I got it working. Tomorrow I will release it :)

jwikman commented 2 years ago

Great David!

Btw, have you seen the new support for Pre-Release? It might be worth looking into, I've added it myself and it's quite straight forward. Even though that there are still some missing pieces on the Marketplace, but it looks nice in VSCode. :)

Read more: https://code.visualstudio.com/updates/v1_63#_pre-release-extensions https://code.visualstudio.com/api/working-with-extensions/publishing-extension#prerelease-extensions

DavidFeldhoff commented 2 years ago

Yes, I've seen that VS Code did something there, but I haven't looked into it deeply yet. And for today I simply created a "real" release as I'm planning to use the days between christmas and new years eve to shut down a bit and don't have to think about lifting up the pre-release to a real release :) The version is now 1.0.15 and I'm curious about your feedback :)

Ah and regarding the performance: I'm not searching for the placeholder values on each text selection as @DavidRoys mentioned. Instead, I start the search for the placeholder values after the user decided to extract the label as it's not mandatory for the code action to show up if the placeholders are found or not, right? So If I find a placeholder value, then I use that of course, but if I can't find any value, then I simply fall back to blank.

jwikman commented 2 years ago

I just can't get this to work... None of these get's any suggeston for %1 and %2:

        Message('Hello %1, %2', "No.", Name);
        Message('%1: %2', FieldCaption(Code), Code);

But I do not know if it is something strange that I am doing...

I even tried with Message('No Local Var Section %1 %3 %2 end.', Customer."No.", Customer."Name 2", Customer.Name); which is what your tests are using, if I'm not mistaken... But no luck there either.

Does they work for you when you do a manual test?

DavidFeldhoff commented 2 years ago

Ah damn it. I ran all my tests locally - everything was fine. I made a few more tests manually and I found one small bug which I've fixed. Unfortunately it was such a fix that broke all other functionality while I did not think that it was possible, so I did not run the tests again and as they are not running in my pipeline yet, I published a not working solution -.- Sorry, for wasting your time with testing version 1.0.15. I've fixed it now. Version 1.0.16 is out and now it's working for me as you can see in my recording. (Wasn't sure if SetFilter is a code cop as well, but if it's the case - then it's working ^^

https://user-images.githubusercontent.com/53570297/147111487-e9bd4391-835a-4010-aa43-97175cabb291.mp4

jwikman commented 2 years ago

Hehe, that explains why it didn't work. :)

Now it works much better, well done!

A few thoughts:

The optimal situation would of course be that we (almost) never need to touch the suggestion made by this function, but it might be hard to get there. I think I'll have to work with this for some time to see if I tend to change the text a lot or so.

ps. I noticed that you've added telemetry, I also did that last week. It's nice to see that the features are actually used!🙂 But a tip: inactivate the performance telemetry, since all users would send performance events every second - totally spamming your application insights...

Now, enjoy the holidays!

DavidFeldhoff commented 2 years ago

No, I don't see a use case for the SetFilter as well. But before implementing the Locked = true, I looked up if it is displayed as code warning and it is not. The only warnings I think I noticed in the past have been these:

And as that's not the case, I don't think that anyone will use the ExtractLabel procedure together with the SetFilter as I always want to see the filter directly, like "<%1" or ">%1", right? That's not something I want to put in a label, just to make it more difficult to read. So I'll leave it as it is as it wouldn't be used anyway, I guess. But prove me wrong if you think different :)

Regarding your second point, I found a solution for that. I'm now only adding the type name (e.g Customer) if the type is a record. For codeunits or other types, I'm not adding it as I think that they don't make sense there. Do you agree? It's released in 1.0.18

https://user-images.githubusercontent.com/53570297/147750344-dc204543-27ad-443f-8e10-de5bf0dd5ecf.mp4

And regarding application insights: Thanks for the advice. My holidays have been a bit about worrying as I will receive a nice invoice of Azure as even after an update (which I did directly) too many already had the old version :/ Well.. sometimes we have to learn some stuff the hard way 🙈

jwikman commented 2 years ago

Thanks David,

I just tested, and it seems to work great!

I agree on the SetFilter, leave it as it is right now. I don't think that it is used that much anyway (but your telemetry might prove me wrong? 😁).

I hope your Azure bill won't be too large! It's insane that the performance telemetry is activated by default, everyone has to learn the hard way...

I think we're ready to close this issue now. Well done!