[FEATURE] control initial number of mages drawn #380
Conversation
Issue: on3iro#379 Amended expedition schema added handling code in the helper and convertExpeditionFromConfig files
|
Let me know if I did this PR right, either here or through discord. And let me know what you think of the changes. I think there are some separation of concerns issues with my current draft of the code, and obviously if a migration is required I haven't done that, but let me know what you think. |
|
@Quicksav cool - thanks for the submission :) I am super busy right now, but I will try to look into this asap (i hope to find time during the upcoming weekend) |
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
please remove these vs-code files :) It would be awesome, if you could add a .gitignore part for vscode. You can probably find something useful here https://github.com/github/gitignore
There was a problem hiding this comment.
(same goes for the .vs/slnx.sqlite file)
| stillAvailableMageIds: mageArray.map(({ id }) => id), | ||
| }).result | ||
| } | ||
| console.log(allMages) |
| .ENG | ||
| const mageArray = Object.keys(allMages).map(function (id) { | ||
| let mage = allMages[id] | ||
| return mage |
There was a problem hiding this comment.
you can just return allMages[id] here instead of making and intermediate assignment :)
| type: 'string', | ||
| }, | ||
| type: 'array', | ||
| $ref: '#/definitions/Array_1', |
There was a problem hiding this comment.
Unfortunately this wont do.
There are a few more things necessary to make all of this work:
We will probably need to:
- Add another type for our
initialBarracksConfigso that it is no longer of the typeBarracksbut the typeInitialBarracksConfigin here https://github.com/on3iro/aer-types/blob/master/types/expeditions.ts
The type will probably look like this:
type InitialBarracksConfig = {
mageIds: [
{ type: 'random' },
{ type: 'custom', id: 'someMage' }
],
supplyIds: string[],
treasureIds: string[]
}- We then need to create a new version of the aer-types package and publish it to npm (I will need to do that) For now, you could change the type locally on your system, to play around a bit until we are satisfied with the API.
- You will need to install the new version with yarn
- You might then see new typescript errors in the compiler - so you will probably need to make adjustments to your code
- You then need to re-create the json-schema with the command inside the comment above
We need to do this, to make the app as typesafe as possible (during compilation as well as at runtime, when users try to import an expedition)
- We also need to change the documentation for initial barracks, so its in line with the change
(7.) It would probably make sense, to also allow treasureIds and supplyIds to be randomly rolled in that scenario. So maybe we should change these in the InitialBarracksConfig as well (which in turn would mean, that you would need to provide additional code for them) - However we can focus on mages now and add that later, if you would prefer to keep this PR streamlined :)
There was a problem hiding this comment.
If you need help here, just let me know. We can also make a call over on discord, so I could guide you through necessary steps, if that would help you :)
There was a problem hiding this comment.
I think a discord call would be good here to show me some of this process. The other issues above are things I can easily amend before our call but the relationship between the aer-types package and the json-schema are something I'm still pretty fuzzy on.
I'm thinking this weekend is the time that makes the most sense to try to get together over discord. I'm also starting to think we might be in pretty different timezones. I live in Arizona so I use MST (Mountain Standard Time). Not sure what time would maybe work for you or if you want to play it by ear.
And I don't mind trying to get supplyids and treasureids working as well.
There was a problem hiding this comment.
Cool - yeah we definitely would have to check timezones before hand :D
I am currently thinking about migrating aer-types back into this repository (but keep publishing it as a separate type-library to npm...). That would make collaborating way easier.
I probably wont be able to make a call during the upcoming weekend because of the timezone issue (this weekend is rather full either) so maybe the weekend after? I will definitely review your code this weekend and leave a few more comments, though. So that way you should be able to already make a few more adjusments. Would that work for you?
(unfortunately you catched me durng a rather busy time with project dead lines at work as well as lots of family stuff to attend to in my spare time)
There was a problem hiding this comment.
(we are nine hours apart btw - so when its 10:00 am in your country its 7:00pm in mine)
There was a problem hiding this comment.
@Quicksav I changed the way this code base works regarding types and data. Both are now part of the repository again (we will still publish them to NPM, so that they can be consumed by other libraries). This will make it way easier to adjust typing while working on a feature, because we no longer need to publish the typings in between etc.
For you to receive these changes, you need to do the following things on your feature branch:
- set a git remote for this original repository (in addition to your own fork upstream remote), name it
upstreamor something like that - than on your feature branch run
git pull --rebase upstream dev- that should get you all the changes you need - run
yarnagain, so that your node_modules are up to date - force push onto your feature branch (rebase alters the git history, hence it's necessary to force push)
Unfortunately I am out of time for now and haven't been able to look at your changes, yet. I will try to do so asap. But if you want you could play around with the typings a bit and try to type the initialBarracksConfig.
If you have any questions, let me know :)
There was a problem hiding this comment.
Having some trouble understanding what I'm supposed to do with this. Unfortunately I think most of my confusion stems from my unfamiliarity with proper git usage. Really just not understanding number 1 of the above steps. Am I supposed to fork the original repository again? Am i just making an 'upstream' branch on my local machine? Or is it that I need to make the 'upstream' branch remote? Sorry for my ineptitude with this I am used to using Visual Studio to handle git commands in a relatively straightforward development environment.
Would having a call next Saturday at ~10AM your time (it'd be Friday after midnight for me) work to go over some of this? I've fixed the other feedback locally but this is the big outstanding thing.
There was a problem hiding this comment.
Hey, no problem :)
So the thing we are trying to achieve is, to get all the new changes made to this repository (so everything which has been merged to dev) into the feature branch you are currently working on.
To do this, you have to follow a few steps.
The first step outlined above, is that you need this repository as a remote source for fetching changes.
Currently your local repository probably only has a single remote (which is probably called origin). A remote is a server you can connect to and push/pull changes/branches/tags etc. from/to. This single remote targets your fork an github. When you type
git remote -v show into your terminal, you will see something like this:
However the url to will point to your forked repository, not our original aeons-end-randomizer repo.
So what you will have to do, is the following (locally):
- run
git remote add upstream [email protected]:on3iro/aeons-end-randomizer.git(note, that if you use https instead of ssh your url will look like this:https://github.com/on3iro/aeons-end-randomizer.git) upstream can be any name here, so you could also name it foo or whatever :) - now you have an additional remote
- now on your feature branch do the following
git pull --rebase upstream dev - this rebases all changes which where made to dev into your feature branch
- Now you need to push all of this to your fork, to update this Pull Request. Because rebase changes the git history, you need to do a force push, so on your feature branch do the following:
git push -f(note: rebasing is very powerful, because it helps keeping a clean git history, however there are a few gotchas, so if you start rebasing in other personal projects, make sure you know these. If you want to know more, I can teach you :) )
Afterwards the PR should be mergable to dev again, and not longer out of date.
Does that help? :)
Edit: Oh, regarding a pair programming session: unfortunately I can't promise that I will have time on saturday. I might be away during the day. We could either schedule something a few hours later (so it would be saturday morning for you and evening for me or maybe do it on sunday)
| @@ -177,7 +177,7 @@ export const getMageIds = ({ | |||
| if (mage === undefined) { | |||
| return baseResult | |||
| } else { | |||
| const ids = mage.ids.filter(idOrRandom => typeof idOrRandom === 'string') | |||
| const ids = mage.ids.filter((idOrRandom) => typeof idOrRandom === 'string') | |||
|
|
|||
| const randomTreasureConfigs = mage.ids.filter( | |||
There was a problem hiding this comment.
we should definitely change the naming here - this is probably a copy and paste issue (i copied some internals from the treasure generation), but its very misleading. -> in turn we should also change it in your code as well.
| } | ||
| } | ||
|
|
||
| export const getMageIdsFromSimpleArray = ({ |
There was a problem hiding this comment.
Hm, this function seems to be almost identical to getMageIds() above. I think we should remove this additional function and use getMageIds() instead.
We could probably type our initialBaracksConfig() in a way, that uses our various rewards config types (e.g. MageRewardsConfig - that way it would be compatible without us doing much.
The only thing we then will have to do, is to write a new migration.
There was a problem hiding this comment.
I had thought about this. I wasn't sure if thats the route we wanted to go because technically its not really a reward, and the rewards config is just a slightly more complex object compared to what I have. But I also see the merit of not having to keep both functions up to date going forward.
There was a problem hiding this comment.
yeah - I think an additional benefit of this would be the consistency for users writing these configs.
|
Hey @Quicksav do you intend to follow up on this? (no pressure - it's totally fine if you currently do not have time for this) |
|
Hey man part of me really wants to get back to this, as I've really been
enjoying using the app for the game and want to do a bit to make it better,
but I think the reality is this is just really back burner for me and I
don't think I'll be getting back to it any time soon.
…On Fri, Mar 5, 2021 at 11:30 AM Theo Salzmann ***@***.***> wrote:
Hey @Quicksav <https://github.com/Quicksav> do you intend to follow up on
this? (no pressure - it's totally fine if you currently do not have time
for this)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#380 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKO3HJVK3S6G47476QBTXRTTCEPLDANCNFSM4R76K4AQ>
.
|
No worries, I can absolutely relate to that. If I have some time in the future and if you don't mind, i might do some refactoring and merge this feature :) |
|
It's highly unlikely that I'll find the time to continue here, therefore I am closing this for now |
Closes: #379
Amended expedition schema added handling code in the helper and convertExpeditionFromConfig files