Skip to content

Depend on nuget packages instead of submodule #64

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

Conversation

martijnhoekstra
Copy link
Collaborator

No description provided.

@martijnhoekstra
Copy link
Collaborator Author

I'm not sure why the sln diff doesn't see any common parts. Maybe line endings? Do you have any specific preference for line endings in the sln file?

@@ -22,7 +22,10 @@ public class Analyzer : IAnalyzer
public Replay Analyze(ReplayFile file)
{
try {
var result = DataParser.ParseReplay(file.Filename, false, false, false, true);
var parseOptions = new ParseOptions() {
AllowPTR = true
Copy link

Choose a reason for hiding this comment

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

So I switched up the parse options from feedback provided by barrret before he merged your changes. So just wanted to make sure you saw those changes. Also, why allowPTR? The uploader doesn’t upload PTR games as far as I am aware

Copy link

Choose a reason for hiding this comment

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

The again, you have to parse PTR to know it’s PTR. So probably why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, that doesn't look right. The old method signature was ParseReplay(string fileName, bool ignoreErrors, bool deleteFile, bool allowPTRRegion = false, bool skipEventParsing = false, bool skipUnitParsing = false, bool skipMouseMoveEvents = false, bool detailedBattleLobbyParsing = false) so that's ignoreErrors = false, deleteFile = false, allowPTR = false, skipEventParseing = true, skipUnitParsing=false, skipMouseMoveEvents = false and detailedBattleLobbyParsing = false. But that's odd: why not skip mouse events. Closing this to look again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you maybe help me out with the versioning? I'm completely lost on where to find which commit is published in which version of the package.

Copy link

Choose a reason for hiding this comment

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

The owner of the repo requested the defaulted state be the same as the previous default state for consistently and reduce confusion. Which makes sense to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have the impression that the build error is related to GitVersion. I asked around at https://gitter.im/GitTools/GitVersion?at=5e29c4d2f85dba0aab003034

Choose a reason for hiding this comment

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

@martijnhoekstra GitVersion is a development-only dependency. It shouldn't be included in or affect the NuGet package: https://github.com/barrett777/Heroes.ReplayParser/blob/master/Heroes.ReplayParser/Heroes.ReplayParser.csproj#L15-L16

Copy link

@barrett777 barrett777 Jan 23, 2020

Choose a reason for hiding this comment

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

I did have issues in development with replays all returning Exception, and I think the issue was different versions of 'sharpcompress'. Do any other projects in this solution use it? I think you need to make sure they are all 0.24.0 or later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A version mismatch of sharpcompress was indeed the issue. It's troublesome that exceptions are swallowed like this to debug this.

@martijnhoekstra
Copy link
Collaborator Author

The .csproj file also has a huge diff where it seems it shouldn't. Also a lineending issue?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants