This project has moved. For the latest updates, please go here.

16513 patch approval

Topics: Developer Forum, Project Management Forum, User Forum
Developer
Jul 6, 2014 at 9:34 PM
Edited Jul 6, 2014 at 10:31 PM
There is a patch published, which contains lot of features. As other projects do, we have development rules. But also we are happy to see any community activity. I am not sure what to do, with patch nr. 16513. I don't want to dismiss it, because i am proud on others work, but there are arguments against and i am not the only one developer here.

Pros:
  • bug fixing
  • new interesting features
  • fits to current implementation
Cons:
  • All in one patch, i.e. bugfixing and features in one patch. More than one task solved in one patch (hard to isolate the related code changes).
  • No unit tests (has to be tested manually)
  • Increases code complexity (but project doesn't have high quality, so it doesn't hurt us)
  • there is refactoring in progress which conflicts with the TSGW improvements
  • features to test are not in our work items list
If author (cant be contacted) splits the patch into separated features and adds unit tests, i have nothing against. What others think about it?
Jul 8, 2014 at 7:59 AM
Hi,

Sorry about the bug fixes + features in the same patch, I'll get them split out over the next day or so and get the bug fixes resubmitted.

So this patch came about because I have a web based tool that contains all of the details about my servers and a browser plugin that allows me to run commands against the servers from my PC. The browser plugin can grab details such as ip / username / password and I wanted to use it to automatically connect to the servers over RDP. This saves me from maintaining 2 inventories (1 in the browser and another in terminals).

Since submitting the patch, there are a few tweaks I want to make (eg: removing the /password: value from the command line after using it to connect to the server which will avoid it being seen in the about dialogue, also not saving the password value in the log), so if you want to go ahead with this feature, I'll submit an updated patch.

Apologies about the lack of unit tests, I didn't see these in the project until I submitted the patch and am not familiar with unit tests in visual studio, I'll have a look at this and create some tests for this feature.


The other big feature in the patch was to put the default TSGW settings in the options dialogue (only used by ad-hoc connections so far, it needs to be integrated into the new favourite dialogue) because I found trying to save default settings quite confusing. I was wondering if other default settings could benefit from being moved into the options dialogue? It would also be useful to make use of the credential manager when configuring passwords (the current TSGW settings for a favourite don't use the credential manager)
Developer
Jul 8, 2014 at 10:01 PM
Some of your ideas are already in the issues tracker. Additionaly you can have a look there and provide the issue numbers, so we can associate them when applying the patch. If you are more interested (it looks like you are able to publish more submits) ask Rob (Project coordinator) for developer permissions. Also It would be nice to have a look at the developer guide: https://terminals.codeplex.com/wikipage?title=Developer%20guide&referringTitle=Documentation
because we use Resharper for code analysis and have some rules. We also have TeamCity to check the project quality trend.

Concerning your submit content:
  • The default options for favorites is poor feature. There already is feature request. Take your extension as temporar solution.
  • I am currently working on refactoring of NewTerminalForm (you can find it as NewTerminalFormCopy) to be able isolate each protocol (see associated change lists). The pesistence is already prepared, so i am expecting, we would be able to reuse credentials selection control everywhere possible.
  • Unit tests have high priority, i am fighting with project code quality, so please learn how to use them using MSTest framework. As you can notice, i already added some of them to partialy verify approved patches.