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

New command: Sync-ALTestCodeunit #25

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

Conversation

martonsagi
Copy link

Hi,

Your PS module helps me a lot with TDD by eliminating the tedious parts, so thanks!

My biggest issue is the need for code re-generation after any kind of scenario updates. Therefore I have added a Sync-ALTestCodeunit command that is currently capable of inserting new features/scenarios into an existing codeunit without deleting any custom code. (This was the most important to me :)

It may also serve as a placeholder to create a full-circle synchronization (#24).

Marton

@lvanvugt
Copy link
Contributor

Hi Marton,

Just now seeing yourinput: wow, that's great. We hadn't had time to pick up #24 yet. Will go and review your suggestions asap.

Luc

@jhoek
Copy link
Contributor

jhoek commented Sep 13, 2019

Great work and very much appreciated, Marton!

There are a few things I would like to discuss with you - I hope that's OK?

  • Maybe we should extract some of the helper methods from your cmdlet class and mine into a separate class, to improve readability/maintainability? Or maybe the classes should have a common (abstract) base class containing these helpers? [postponed for now]
  • I'm even thinking that your cmdlet and mine could be the same thing: should it matter whether your starting point is an empty file or an existing AL codeunit? PowerShell modules are an API for script authors, so this is not something that is easily changed later. [postponed for now]

A few questions about the code:

  • You made InitializeFunction private, and no longer a cmdlet parameter. Should we get rid of it entirely? [remove]
  • Should the CodeunitPath parameter not be positional? Most essential path parameters in PowerShell cmdlets are? [make positional]
  • Especially since you derive from PSCmdlet, I think the CodeunitPath should be resolved using something like GetUnresolvedProviderPathFromPSPath. Your current approach does not handle PS constructs that .NET is not aware of, like PowerShell drives or e.g. '~\Desktop'. [resolve path]
  • Would it not confuse users that the original cmdlet writes strings to the pipeline, whereas yours manipulates files? [will leave this for now]
  • Even if we leave it like that, I think your list of new scenarios and helpers (line 88/89) should probably be sent to another stream, e.g. the verbose or information stream? [will do this]
  • A cmdlet that updates a file is more useful if it returns the FileInfo for the updated file, so that the next cmdlet in the pipeline can use that for further processing - would you agree? [add]
  • I guess we both should add an [OutputType()] attribute to our cmdlets... [add]

Looking forward to hearing from you!

Notes to self:

  • Consider renaming cmdlet to Update-... instead of Sync-...
  • Add contributor names to manifest
  • Superfluous blank lines before and after test and helper functions
  • Feature tag for new features is not added to the file header

@jhoek
Copy link
Contributor

jhoek commented Sep 28, 2019

@martonsagi Luc and I would like to suggest that we apply the changes in square brackets in the comment above before merging in your pull request. From what I understand, this requires that you check the "Allow Edits from Maintainers" checkbox (https://stackoverflow.com/questions/20928727/adding-commits-to-another-persons-pull-request-on-github) in your pull request. Could you please confirm that you have done so?

@martonsagi
Copy link
Author

@jhoek I'm not sure if I clicked that checkbox. I've added you as collaborator to my repo, hope that helps. Sorry for the long pause, I'm also hoping to have some time this weekend to write a proper reply.

@jhoek jhoek mentioned this pull request Oct 4, 2019
@martonsagi
Copy link
Author

Ok, so I finally have the time for a proper reply.

  • Maybe we should extract some of the helper methods from your cmdlet class and mine into a separate class, to improve readability/maintainability? Or maybe the classes should have a common (abstract) base class containing these helpers? [postponed for now]

This was my first thought when I finished the new cmdlet that almost 80% of it is identical so that could be better organized. If only I had more time then... :)

  • I'm even thinking that your cmdlet and mine could be the same thing: should it matter whether your starting point is an empty file or an existing AL codeunit? PowerShell modules are an API for script authors, so this is not something that is easily changed later. [postponed for now]

I think so too that ultimately it should be one cmdlet, I created a seperate class to demonstrate to possibilities.

A few questions about the code:

  • You made InitializeFunction private, and no longer a cmdlet parameter. Should we get rid of it entirely? [remove]

I believe the functionality of InitializeFunction is useful and should remain a part of the generated code itself. I'm not sure if this is something I would ever set to false, hence the reason I changed it to a private property.

  • Should the CodeunitPath parameter not be positional? Most essential path parameters in PowerShell cmdlets are? [make positional]
  • Especially since you derive from PSCmdlet, I think the CodeunitPath should be resolved using something like GetUnresolvedProviderPathFromPSPath. Your current approach does not handle PS constructs that .NET is not aware of, like PowerShell drives or e.g. '~\Desktop'. [resolve path]
  • Would it not confuse users that the original cmdlet writes strings to the pipeline, whereas yours manipulates files? [will leave this for now]

These are the parts of PowerShell magic that I would gladly leave to you for decision. This was literally my first PS CmdLet.:)

  • Even if we leave it like that, I think your list of new scenarios and helpers (line 88/89) should probably be sent to another stream, e.g. the verbose or information stream? [will do this]

Agree, it was just easier for me to debug at the time.

  • A cmdlet that updates a file is more useful if it returns the FileInfo for the updated file, so that the next cmdlet in the pipeline can use that for further processing - would you agree? [add]
  • I guess we both should add an [OutputType()] attribute to our cmdlets... [add]

Again, PS Magic, as you think it's the best way.


Now that I've stated I have almost no idea about PS Cmdlet API :), I wanted to bring in another contribution point that may worth opening a separate issue.

I've been thinking about the full-circle synchronization and how to create a solution that could do a bit more than inserting new test cases. This functionality worth having a backend library that the PS Cmdlets are able to utilize, therefore issues with repetitive functions can also be eliminated.

I have created a new library that is able to extract TestFeature/TestScenario metadata from AL Codeunit files, and also writing it back into files.
The library gives us the ability to work with test files in a proper OOO manner.
https://github.com/dynasist/ALObjectParser

I hope a .NET Core 3.0 DLL is compatible with ATDD.TestScriptor project. Maybe this is something we could also work on to integrate into this PS module?

Example: parsing Dept_TableTests.al which was generated with ATDD.TestScriptor :)

> .\ALObjectParser.Cmd.exe "D:\xxx\DynAsist\bc-departments\DepartmentsTest\tests\Dept_TableTests.al"

Object info: codeunit 89912 "Dept_TableTests_DSK"
Object path: D:\xxx\DynAsist\bc-departments\DepartmentsTest\tests\Dept_TableTests.al
-----------------------------------------------------------
Test Feature: Department Item Table including 2 scenario(s)

  Test Scenario: #1 Check OnBeforeInsert Event
    GIVEN: A Menu Item
    GIVEN: Parent Guid is empty
    WHEN: Insert new record
    THEN: Main Menu flag populated

  Test Scenario: #2 Check OnBeforeModify Event
    GIVEN: A Menu Item
    GIVEN: Parent Guid is empty
    WHEN: Existing record changed
    THEN: Main Menu flag populated

-----------------------------------------------------------
Test Feature: Department Suite Table including 1 scenario(s)

  Test Scenario: #1 Check OnBeforeDelete Event
    GIVEN: A Suite
    GIVEN: Has Menu Items
    WHEN: Record is deleted
    THEN: Menu Items deleted

-----------------------------------------------------------

@jhoek
Copy link
Contributor

jhoek commented Oct 7, 2019

@martonsagi Really like that ALObjectParser idea! If Luc agrees, I would like to look into that some more before deciding whether or not to invest time in the original approach. Do you think we could organise a short Skype call so that you can demo your idea and we can discuss it some more? @lvanvugt Are you in? :-)

@jhoek
Copy link
Contributor

jhoek commented Oct 7, 2019

@martonsagi Full disclosure: I have taken some months off work to try and build a (potentially commercial) product for AL source code manipulation. I don't expect much functional or technical overlap, but it that's a concern for you, please let me know.

@lvanvugt
Copy link
Contributor

lvanvugt commented Oct 7, 2019

Count me in

@martonsagi
Copy link
Author

@jhoek I'm glad you like it. :) Sure, Skype/Teams meeting works for me. Invited you on LinkedIn to discuss the details.
Commercial usage does not concern me. What's under MIT that remains under MIT license. :)
I'm planning to re-implement the processing layer of AL Object Designer in .NET, since NodeJS proved to be dead-slow with that many symbols.

@jhoek
Copy link
Contributor

jhoek commented Oct 17, 2019

@martonsagi As mentioned in our Skype call, my own project and your choice of license may be a good reason for me not to get heavily involved in your project. @lvanvugt will contact you later, but my latest idea was for me to just build the PowerShell-interface on top of your project, after some minor restructuring. I'll add my suggestions as issues on your GitHub project page for you to consider.

@martonsagi
Copy link
Author

@jhoek Sound fine for me! I'll review the proposals and continue the discussion there.

@lvanvugt
Copy link
Contributor

@lvanvugt will contact you later, but my latest idea was for me to just build the PowerShell-interface on top of your project, after some minor restructuring. I'll add my suggestions as issues on your GitHub project page for you to consider.

As written on #5 I have (and still am) very busy with conducting courses. I will contact @martonsagi asap.
Thanx @jhoek for stepping in already with the above note (and quoted here).

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.

3 participants