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

Enable item atcommand to have optional parameters #2795

Merged
merged 2 commits into from
Jul 26, 2020

Conversation

Emistry
Copy link
Member

@Emistry Emistry commented Jul 5, 2020

Pull Request Prelude

Changes Proposed

Issues addressed:
These parameters are actually optional, use default value if not provided.
So that we don't really need to enforce 0 for these fields if not required (example card fields 2 ~4)
However, if the fields that we wanted to customize a certain parameter, then all the parameters before it required to set value.

Instead of

@item2 5001 1 1 10 0 4001 0 0 0

we could just do these

@item2 5001
@item2 5001 1 1 10                     (refine only)
@item2 5001 1 1 10 0 4001              (refine + 1 card)
@item2 5001 1 1 10 0 4001 4002         (refine + 2 cards)
@item2 5001 1 1 10 0 4001 4002 4003    (refine + 3 cards)

@Emistry Emistry added the type:enhancement Issue describes an enhancement or feature that should be implemented label Jul 5, 2020
@4144
Copy link
Contributor

4144 commented Jul 6, 2020

hard to read changes.
probably you can split actual changes and style fixes into separate commits?

- update code format
- these parameters are optional for item creation, could just use
  default values.
@MishimaHaruna MishimaHaruna merged commit ccf375c into HerculesWS:master Jul 26, 2020
@MishimaHaruna MishimaHaruna added this to the Release v2020.07.26 milestone Jul 27, 2020
@Emistry Emistry deleted the atcommand_item branch August 24, 2020 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants