-
Notifications
You must be signed in to change notification settings - Fork 163
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
Hide Tags from Living documentation #516
Conversation
* Pickles is unable to deal with Danish characters (picklesdoc#477) * Version 2.16.2
but keep feature
Hi - sorry for not reacting earlier. I was ill last week, and on a training next week. So I will need a bit more time to review this in detail. I do have one question right away: what is - in your opinion - the definition of a "technical tag"? |
Hello, no worries, i'm patient. |
Hi @spacehole1, I've had a look at your pull request. It looks solid so far. However, I don't see changes to the .xaml ui file. You added the new property to the ViewModel, but not to the actual view. I'm still not convinced of the business value of this feature. Is BDD acceptance suffering in your team as a result of those technical tags being shown? In any case, if I decide to add this functionality, I would like to rename the property into something that more clearly distinguishes it from the Exclude Tags functionality. "Exclude Tags" excludes scenarios with certain tags, but "Technical Tags" doesn't really say what it does. Do you have a suggestion for a name that more clearly expresses the intention? |
Hello, Sorry for the xaml, it's fixed. I suggest HideTags, it should clearly explain what it does. The problem of those tags for my team is not for BDD acceptance. Its more that they are confusing tags for business. They don't understand what are the meaning of those tags. |
@@ -260,7 +260,14 @@ public Feature MapToFeature(G.GherkinDocument gherkinDocument) | |||
|
|||
foreach (var tag in gherkinDocument.Feature.Tags) | |||
{ | |||
feature.AddTag(this.MapToString(tag)); | |||
var stringTag = this.MapToString(tag); | |||
var hideTags = string.IsNullOrEmpty(configuration.HideTags)?null:configuration.HideTags.Split(';'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detail: you can declare and initialize hideTags outside of the foreach loop.
feature.AddTag(this.MapToString(tag)); | ||
var stringTag = this.MapToString(tag); | ||
var hideTags = string.IsNullOrEmpty(configuration.HideTags)?null:configuration.HideTags.Split(';'); | ||
if (hideTags != null && hideTags.Any(t => t.Equals($"@{stringTag}", StringComparison.InvariantCultureIgnoreCase))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more crucial thing is that you are doing the filtering of the tags only on the feature level. Scenarios will still show the tag you are trying to hide.
If we introduce this feature, I would expect the tags to be hidden from scenarios as well.
Oh, and please update your development branch with the latest code from the development branch. There have been some changes that fix weird compilation errors that turn up from time to time. |
# Conflicts: # src/Pickles/Pickles.CommandLine/NLog.xsd # src/Pickles/Pickles.UserInterface/NLog.xsd
Just changed according to your remarks. |
@@ -190,21 +190,23 @@ public Location MapToLocation(G.Location location) | |||
return location != null ? new Location { Column = location.Column, Line = location.Line } : null; | |||
} | |||
|
|||
public Scenario MapToScenario(G.Scenario scenario) | |||
public Scenario MapToScenario(G.Scenario scenario, string[] tagsToHide) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more elegant to make the tagsToHide parameter a params array (params string[] tagsToHide). That would remove the need to pass in null arguments on many calls of the method.
@@ -224,13 +226,15 @@ public Example MapToExample(G.Examples examples) | |||
}; | |||
} | |||
|
|||
public ScenarioOutline MapToScenarioOutline(G.ScenarioOutline scenarioOutline) | |||
public ScenarioOutline MapToScenarioOutline(G.ScenarioOutline scenarioOutline, string[] tagsToHide) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more elegant to make the tagsToHide parameter a params array (params string[] tagsToHide). That would remove the need to pass in null arguments on many calls of the method.
Thanks for continuing to work on this. I'll be on holiday until end of May, so I won't be able to review further changes until then. |
@dirkrombauts good remark. Now should be better |
Thank you for your contribution, and thank you for your patience! |
Released in version 2.19.0 |
Hello,
Some tags added to the specflow only have a technical meaning to run the tests, but shouldn't be shown to the users.
I propose to added a new parameter : technical Tag allowing user to remove tags from the pickle generated files.
This should ease the use of tags, and have a more useful testing result report :
It should only show functionnal tags.
Regards,
PS : The documentation update is not forgotten and will come shortly if the change is good for you