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

Exclude files when previously excluded and no wildcards are in play #191

Merged
merged 3 commits into from
Sep 7, 2018

Conversation

hvanbakel
Copy link
Owner

It's not pretty because of the wildcard support we currently have. But if the user is not using a wildcard include then we can generate the appropriate exclusions. Having a wildcard include is probably an exception anyway as it was never really supported in any UI and the current wildcard include is automatic.

@hvanbakel hvanbakel changed the title More excludes Exclude files when previously excluded and no wildcards are in play Sep 6, 2018
@hvanbakel
Copy link
Owner Author

I actually thought we were logging this but we must've removed that when wildcards were added.

@andrew-boyarshin
Copy link
Collaborator

andrew-boyarshin commented Sep 6, 2018

No, it was I who removed that in my IncludeItems refactoring PR, since:

  1. It was too verbose (and we didn't have verbosity switch at that time)
  2. It required passing additional ILogger argument.

upd: will do review in ~8 hours.
upd: missed timeframe for both PRs, next 9 hours it is.

@hvanbakel
Copy link
Owner Author

That's fine, I don't think we need to get that back, it's just that I hadn't noticed it being gone.

Copy link
Collaborator

@andrew-boyarshin andrew-boyarshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor naming and codestyle issues (can be fixed later), no actual issues though.

@@ -88,6 +107,11 @@ public void Transform(Project definition)
logger.LogInformation($"Removed {count} include items thanks to Microsoft.NET.Sdk defaults");
}

private static IEnumerable<string> PreviouslyExcludedFiles(Project definition, string[] removedIncludes)
{
return definition.ProjectFolder.GetFiles("*." + definition.CodeFileExtension, SearchOption.AllDirectories).Select(x => x.FullName.Substring(definition.ProjectFolder.FullName.Length + 1)).Except(removedIncludes);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a long line.

.Where(x => x.EndsWith("." + definition.CodeFileExtension, StringComparison.OrdinalIgnoreCase))
.ToArray();

var wildcardIncludes = keepItems.Where(x => x.Name.LocalName == "Compile").Select(x => x.Attribute("Include")?.Value).Where(x => x != null && x.Contains("*")).ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a long line.

.Where(x => !string.IsNullOrEmpty(x))
.ToArray();

otherIncludeFilesMatchingWildcard = otherIncludeFilesMatchingWildcard.Union(PreviouslyExcludedFiles(definition, removedIncludes)).ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a long line.

{
var removedIncludes = removeQueue
.Where(x => x.Name.LocalName == "Compile")
.Select(x => x.Attribute("Update")?.Value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhmm, should this be Remove instead of Update?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strike that. Although removedIncludes is not a good name now that I think of it, brings ambiguity to this code block. Is referencedItems better?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll apply the rename in my merge

@hvanbakel
Copy link
Owner Author

I don't know exactly what happened but can someone check what happened in:
beeea00
5e744ff
e32f8e8

I believe I reverted the commit correctly....

@andrew-boyarshin
Copy link
Collaborator

@hvanbakel no, bad stuff happened. You reverted what shouldn't be reverted. I'll fix this now.

@andrew-boyarshin
Copy link
Collaborator

andrew-boyarshin commented Sep 14, 2018

@hvanbakel I have rewritten git history and dropped merge of wrong branches and following wrong revert (in fact, you've additionally reverted 2 PRs). History is now linear without that mistake. To fix git repo locally:

> git fetch
> git reset --hard origin/master

For posterity (or those willing to find out why 3.0.1 tag is on commit that is not in master branch git history as it was) my force push output:

 + 7228783...c972c15 master_fix -> master (forced update)

@hvanbakel
Copy link
Owner Author

Awesome @andrew-boyarshin this is all still very new to me (fixing branches/wrong merges) so thanks for being the seasoned git person 👍

So from looking at this again, I don't think I should've done the revert to begin with, it was fine, right?

@andrew-boyarshin
Copy link
Collaborator

@hvanbakel it was not fine, but was acceptable. Without revert it would preserve all changes though.

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.

2 participants