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

Benchmarks #10

Merged
merged 8 commits into from
May 17, 2021
Merged

Benchmarks #10

merged 8 commits into from
May 17, 2021

Conversation

gfoidl
Copy link
Contributor

@gfoidl gfoidl commented May 17, 2021

Fixes #2
Based on top of benchs added in #1

@gfoidl gfoidl requested a review from BenjaminAbt May 17, 2021 15:43
@BenjaminAbt
Copy link
Member

The Benchmark project is being created as nuget and published.
We should exclude it.

using DeviceDetectorNET;
using MyCSharp.HttpUserAgentParser.Providers;

namespace MyCSharp.HttpUserAgentParser.Benchmarks.LibraryComparison
Copy link
Member

Choose a reason for hiding this comment

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

we should move the copied source of UserAgent in the current benchmark project too to compare without caching

@BenjaminAbt
Copy link
Member

CI is not triggered, but why?

fix trigger
@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

The Benchmark project is being created as nuget and published.
We should exclude it.

What's the easiest way? I can't see the forest of the trees at the moment.

@BenjaminAbt
Copy link
Member

done

@BenjaminAbt
Copy link
Member

i have a better idea

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

What do you think about 5b8213f?

I verifyed it works as it should.

@BenjaminAbt
Copy link
Member

I think the explicit behavior is easier to understand / manage if we use this in other projects etc

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

Makes, sense, I'll revert this one.

@gfoidl gfoidl merged commit d2c4a82 into main May 17, 2021
@gfoidl gfoidl deleted the benchmarks branch May 17, 2021 17:58
@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

Damned, I didn't look enough, now it's a merge-commit instead a squash-merge (for linear history).
The defaults did change...re-enabled squash-merging now.

@BenjaminAbt
Copy link
Member

Nooo, squad marge does not work with NerdBank GitVersioning!

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

We have this in the other repos too?
Then NerdBank GitVersioning is faulty, as squash merging should be the way to merge PRs for linear history which is much cleaner to follow (and bisect in case of). Merge commits for PRs bring every intermediate commit of a PR into main, but these should be a detail.

@BenjaminAbt
Copy link
Member

Yes, we had a large discussion in other repos and decided to not use squash merge

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

decided to not use squash merge

I missed that (can remember the discussion, but not a conclusion), and I'm not fine with not using squash merge. Merge commit ruin the history.
If NerdBank does work only with merge commit it sucks 😢

@BenjaminAbt
Copy link
Member

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

Just read the same issue.
There the NerdBank also sucks. IMO this isn't a solution for versioning, as it only works half-ways.
Do we need to publish a package for each CI build? Who will be using these packages anyway? I'd prefer a package built from main and pushed, and for PRs (to validate it's usage in another project) create some kind of preview (manually).

@BenjaminAbt
Copy link
Member

for all I care, we can only ship stable packages

@gfoidl
Copy link
Contributor Author

gfoidl commented May 17, 2021

👍🏻 agree.

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.

Add benchmarks and compare to other libraries
2 participants