Skip to content

Move Concordion.NET to .NET Standard and NUnit v3 #27

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

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

Conversation

KidFashion
Copy link

This is a first step at moving legacy concordion.net baseline to .NET Standard and NUnitv3.

Currently code compiles and tests succeed but we need to rebuild NUnit integration because NUnitv2 AddIn has been completely replaced in NUnitv3 as explained here.

@ShaKaRee
Copy link
Member

ShaKaRee commented Oct 1, 2017

Hello Angelo,

thank you very much for contributing to the Concordion.NET project. Your help is highly welcome.

Before, we include your changes to the project, could we clean it up a bit?

  • Could you remove "CAZZONE" from the source code?
  • Could you remove all debugging code such as lines with Console.WriteLine?

Cheers

@KidFashion
Copy link
Author

Hello ShaKaRee,

Thank you for spotting those issues, I've now cleaned up a bit, keep in mind two things:

  • I've removed Console.WriteLines added by me (that were already commented out) but kept Console.WriteLines already present in code before (if that's a problem just make me know).
  • This version is not intended to be released as it is, as discussed in Concordion Slack I worked on it this summer and now I've problems in finding more time to work on it so it was suggested me to open a pull request in order to start sharing work with others. If you prefer to add it to a nunit3 branch or whatever instead of targeting master I'm more than happy to redirect the PR there.

@ShaKaRee
Copy link
Member

ShaKaRee commented Oct 7, 2017

Hello Angelo,

great that you share your work as soon as possible with others. I would like to include it directly into the master branch and I am looking forward to see the NUnit3 integration growing. :-)

Thank you for cleaning up the code.

Could you do a final step before we include it to the main branch? Could you remove the .old files? I think git provides a great history so that we can recover the old files easily.

Cheers

@nigelcharman
Copy link
Member

Hi, I'm in 2 minds about pulling to master:

  1. master should always be in a releasable state. My understanding is that this code breaks the NUnit integration.
  2. The concordion-net project isn't live. This is a step forward that takes us towards a working version of NUnit 3 code.

Keen to understand your thoughts on merging to master, @KidFashion, @ShaKaRee?

@nigelcharman
Copy link
Member

Also, should we set up CI for this branch? Using AppVeyor or something else?

@nigelcharman
Copy link
Member

Finally, can we clearly comment on the concordion.net and concordion-net projects, so users can understand the status of each project?

@KidFashion
Copy link
Author

@ShaKaRee: I've removed the .old files.
@nigelcharman: Current code does not break NUnit integration, I started by making the current version compilable on .NET Core (this means with a working NUnit2 integration) and then I created a new Concordion.NUnit3 project where I started working on NUnit3 integration.

This means that we're not breaking NUnit2 but you will have an half finished (and not working) version of NUnit3 support, this is why I was asking if it was better to target a different branch from master.

I think we should have a CI for each branch and AppVeyor works fine for me.

@ShaKaRee
Copy link
Member

Hi guys,
what about mergin the start of NUnit 3 integration into the main project?
@KidFashion perhaps you will find some time in near future to finish it. :-)
Cheers

@KidFashion
Copy link
Author

@ShaKaRee Indeed, I started it during summer holidays, so Christmas holidays seems a nice period to complete it :)

@nigelcharman
Copy link
Member

@KidFashion any chance of an update on this PR? I've mentioned it in the discussion about future .NET direction at concordion/concordion.net#15.

@KidFashion
Copy link
Author

@KidFashion any chance of an update on this PR? I've mentioned it in the discussion about future .NET direction at concordion/concordion.net#15.

Unfortunately I've still to find time to look into that, since you pinged me I could look into that in the next two weeks but can't promise... :(

@jamiehankins
Copy link

jamiehankins commented Jun 30, 2020

There are a few bugs relating to PRE tags that are causing us issues.

  1. The parser gobbles up spaces between SPAN tags if space is all there is.
  2. If you use an " ", the parser throws an exception. Instead, you have to use " " (ugly!)
  3. The parser includes spaces at the beginning of the PRE object for indenting, even though PRE content is supposed to be left alone.

I'm guessing that fixing any of this is out of the question with this PR hanging over things.

@nigelcharman
Copy link
Member

@jamiehankins as you might have guessed this PR has stalled.

Would you raise your bugs as issues please, with a test case to show them? I'd like to see how the Java version of Concordion behaves with the same spec.

I suggest we then reach out to our users to see if anyone has the bandwidth to fix them.

@jamiehankins
Copy link

jamiehankins commented Jul 8, 2020

@nigelcharman done as issues #28 #29 and #30

@nigelcharman
Copy link
Member

Thanks @jamiehankins , can you confirm which code base you are using? Is it concordion.net or concordion-net? If the latter, have you applied this PR? Just so we can be clear to anyone wanting to contribute fixes. Thanks

@jamiehankins
Copy link

@nigelcharman Oops, I missed your last message. I'm no longer with the company I was working for, so I don't know the answer to your main question. The answer to the second question is no, we never applied the PR.

I have a job offer that I'm likely to accept later today. The job involves a lot of C#, so I might evangelize Concordion.NET. If that works out, maybe I can get buy-in to work on this.

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.

4 participants