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

Trade filter #358

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

Trade filter #358

wants to merge 13 commits into from

Conversation

mchvn
Copy link

@mchvn mchvn commented Feb 9, 2021

No description provided.

Copy link
Member

@dumbmatter dumbmatter left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you for the PR!

I left a bunch of comments. I hope they don't come off as too negative or discouraging, that is not my intention!

At any time, you are free to tell me that you're done working on it and you want me to take it from here. So you don't need to address all of my comments if you don't want to deal with it. But if you do want to address them all, that'd be great too :)

Also, I forget if I mentioned this to you before, but I do need you to sign the CLA before I can merge a PR https://github.com/dumbmatter/gm-games/blob/master/CLA-individual.md - basically that just gives me ownership of code you contribute to the project. The reason for that is I wrote 99.9% of BBGM, and I don't want to deal with the legal complexity of having it partially owned by many different people. And also make sure you're okay with the non-standard license this code is under https://github.com/dumbmatter/gm-games/blob/master/LICENSE.md - that exists just because of the business model of a free single player game. If I had not yet mentioned this to you, I apologize, I should have.

color: $black;
border-color: $primary;
}
.color-primary {
Copy link
Member

Choose a reason for hiding this comment

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

This is the same as the text-primary class built into Bootstrap.

Possibly those other classes could be replaced by something in Bootstrap too, not sure...

import type { RatingKey } from "./types.basketball";

const SKILLS: Skill = {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to instead add a "description" field to COMPOSITE_WEIGHTS? Then there's never any concern that SKILLS and COMPOSITE_WEIGHTS could fall out of sync.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I'll change it to that approach. We'll just have to call a mapping function to extract the skill labels/descriptions (iirc) when we want them. Couldn't decide which way to go with it between the two

S: "Sniper",
},
});
const tooltips = SKILLS;
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see this gone :) for the same reason I wrote above, this old code of mine was bad because it required the list of skills here to remain in sync with the list of skills in COMPOSITE_WEIGHTS, so it was just asking for a bug to appear..

@@ -485,6 +531,7 @@ const DEFAULT_DIVS: Div[] = [
const DEFAULT_STADIUM_CAPACITY = 70000;

export {
SKILLS,
AWARD_NAMES,
DEFAULT_CONFS,
DEFAULT_DIVS,
Copy link
Member

Choose a reason for hiding this comment

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

In addition to constants.basketball.ts and constants.football.ts, there's now also constants.hockey.ts :)

Exciting times...

label: "A",
description: "Athlete",
},
};
const COMPOSITE_WEIGHTS: CompositeWeights<RatingKey> = {
Copy link
Member

Choose a reason for hiding this comment

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

Very recently a "V" "Volume scorer" label was added to "scoring", looks like it got lost in a merge.

Copy link
Author

Choose a reason for hiding this comment

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

I also just realized I seem to have caught you guys as hockey is being added. I'll go ahead and make sure those are included as well

@@ -0,0 +1,19 @@
export default class FilterItem {
filterData: any; //filter items selected, ex for Pos: C, PF...
filterFunction: string; //function that will decide if the offer qualifies given list of players
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more clear to accept the actual function here, rather than a string identifier that appears in some other file.

Copy link
Author

Choose a reason for hiding this comment

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

So that was my original intent, but the worker seems to be pretty strict on what it accepts, i.e. it specifically precludes functions from what I can tell. Let me know if I'm missing something though!

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I didn't realize this was being called from the ui. Everything sent between goes through the "structured cloning" algorithm which does not let you easily pass functions.

@@ -0,0 +1,19 @@
export default class FilterItem {
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, make the file name FilterItem.ts, if it just exports one class FilterItem.

@@ -0,0 +1,19 @@
export default class FilterItem {
filterData: any; //filter items selected, ex for Pos: C, PF...
Copy link
Member

Choose a reason for hiding this comment

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

Don't use any here, or in any (ha) of the other functions in this class. I know you have some cases where this is an array and others where it's a string, but that is a classic example of when a generic should be used.

Copy link
Author

Choose a reason for hiding this comment

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

you're right! like I said, still new TS so I appreciate you bearing with me as I learn

//since filters reduce the number of viable offers, try up to 10 times to get some good offers
let filteredOffers: OfferType[] = [];
let iterations = 0;
while (filteredOffers.length === 0 && iterations < 10) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for this to result in your getting the same (or very similar) offers twice? I don't see a team restriction anywhere.

Copy link
Author

Choose a reason for hiding this comment

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

Are you referring to subsequent offer requests? Can you elaborate? The filters are applied as the final step, so that aspect shouldn't be any different from how offers initially worked

Copy link
Member

Choose a reason for hiding this comment

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

Previously getOffers was called once. Now it's called up to 10 times. But each time it's called, some of the offers returned could be duplicates of previous ones, right?

Copy link
Author

Choose a reason for hiding this comment

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

So if it gets any offers, it will return them (even if just 1) and display them to the player. Definitely possible/likely that it asks a team multiple times-- but sometimes the offer will be slightly different and may end up meeting the conditions that a previous one didn't

@@ -91,6 +94,24 @@ import {
} from "../core/season/awards";
import { getScore } from "../core/player/checkJerseyNumberRetirement";
import type { NewLeagueTeam } from "../../ui/views/NewLeague/types";
import * as filter_functions from "../core/trade/filterFunctions";

export type OfferType = {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could save having to manually write out this type with something like type OfferType = ThenArg<ReturnType<typeof augmentOffers>>; Also it should be called Offer or AugmentedOffer rather than OfferType - we already know it's a type :)

Copy link
Author

Choose a reason for hiding this comment

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

yeah, still pretty new to TS so I'm still winging it a bit here and there hah-- will fix

Copy link
Author

Choose a reason for hiding this comment

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

I believe we can't reference augmentOffers, since it's scoped in getTradingBlockOffers. Let me know if you think we should extract that function

@maxkarnold
Copy link

@dumbmatter So if I wanted to continue working on this PR how would I do that. Would I need to create a new pull request? Or somehow get permission from @mchvn to use what he has currently worked on and work off that? Not sure how to move forward.

@dumbmatter
Copy link
Member

@dumbmatter So if I wanted to continue working on this PR how would I do that. Would I need to create a new pull request? Or somehow get permission from @mchvn to use what he has currently worked on and work off that? Not sure how to move forward.

I'm not sure... a lot of things have changed since this PR was made, so it might take some work to merge the latest commits. Then you'd need to address the issues with it. Probably in a new PR based on this.

Or, it might be easier to start from scratch. Maybe influenced a bit by this code.

@maxkarnold
Copy link

@dumbmatter So if I wanted to continue working on this PR how would I do that. Would I need to create a new pull request? Or somehow get permission from @mchvn to use what he has currently worked on and work off that? Not sure how to move forward.

I'm not sure... a lot of things have changed since this PR was made, so it might take some work to merge the latest commits. Then you'd need to address the issues with it. Probably in a new PR based on this.

Or, it might be easier to start from scratch. Maybe influenced a bit by this code.

Sounds good, I'll keep this PR as a reference and take a look at where we can use this code.

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.

3 participants