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

Fix map linting crashing for not existing directories #14136

Merged
merged 4 commits into from Oct 8, 2017

Conversation

abcdefg30
Copy link
Member

Fixes the regression that slipped in #13938 in (see #13938 (comment)).

For reference: https://logs.openra.net/?year=2017&month=08&day=30#15:42:38

@abcdefg30 abcdefg30 force-pushed the cacheMaps branch 2 times, most recently from 7e011ab to 66252c1 Compare October 7, 2017 10:00
{
var name = kv.Key;
var classification = string.IsNullOrEmpty(kv.Value)
? MapClassification.Unknown : Enum<MapClassification>.Parse(kv.Value);
Copy link
Member

@phrohdoh phrohdoh Oct 7, 2017

Choose a reason for hiding this comment

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

Enum.TryParse and continue upon failure or Unknown.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore this I suppose. You're just moving code afterall.

Copy link
Member

Choose a reason for hiding this comment

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

This would be a good opportunity to fix the original code (since this is pulling out a common helper).

@@ -78,42 +77,19 @@ void IUtilityCommand.Run(Utility utility, string[] args)
// The map cache won't be valid if there was a map format upgrade, so walk the map packages manually
// Only upgrade system maps - user maps must be updated manually using --upgrade-map
Console.WriteLine("Processing Maps:");
foreach (var kv in modData.Manifest.MapFolders)
var maps = modData.MapCache.EnumerateMapsWithoutCaching();
Copy link
Member

Choose a reason for hiding this comment

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

Why store this in a local?

@abcdefg30
Copy link
Member Author

Updated.

reaperrr
reaperrr previously approved these changes Oct 8, 2017
@@ -122,6 +122,48 @@ public void LoadMaps()
}
}

public IEnumerable<Map> EnumerateMapsWithoutCaching(MapClassification classification = MapClassification.System)
Copy link
Member

Choose a reason for hiding this comment

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

For this to work properly you will need to change the MapClassification definition to:

// Used for grouping maps in the UI and for filtering
[Flags]
public enum MapClassification
{
	Unknown = 0,
	System = 1,
	User = 2,
	Remote = 4
}

if (!Enum.TryParse(kv.Value, out packageClassification))
continue;

if (packageClassification != classification)
Copy link
Member

Choose a reason for hiding this comment

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

This will then need to become

if (!classification.HasFlag(packageClassification))

}
}
// Use all system maps for lint checking
maps = modData.MapCache.EnumerateMapsWithoutCaching().ToList();
Copy link
Member

@pchote pchote Oct 8, 2017

Choose a reason for hiding this comment

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

This is silently changing the command behaviour.

Please do EnumerateMapsWithoutCaching(MapClassification.System | MapClassification.User) and then remove the User in its own commit to make the intention clear.

}
}
}
Console.WriteLine(map.Package.Name);
Copy link
Member

Choose a reason for hiding this comment

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

Left over debugging code?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional. We also print out which yaml files are currently upgraded. It also takes quite a while to update all maps, and you're left without knowing if something even happens.

{
// Assume that the path is a directory if there is not an existing file with the same name
var resolved = Platform.ResolvePath(name);
if (!File.Exists(resolved))
Copy link
Member

@pchote pchote Oct 8, 2017

Choose a reason for hiding this comment

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

The support directory is not a file, so this always fails. This needs to also check Directory.Exists. The comment also needs to be corrected or removed.

@abcdefg30
Copy link
Member Author

Updated.

@pchote pchote merged commit 0727f7e into OpenRA:bleed Oct 8, 2017
@abcdefg30 abcdefg30 deleted the cacheMaps branch October 8, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants