dr-scripts icon indicating copy to clipboard operation
dr-scripts copied to clipboard

[script][common-crafting] New method added to handle checking for sigils and purchasing, if it can.

Open Dartellum opened this issue 2 years ago • 14 comments

Dartellum avatar Aug 23 '22 22:08 Dartellum

Leaving guild planned sigils out for now.

Dartellum avatar Aug 24 '22 20:08 Dartellum

Dart, this is a little bit picky but I think it greatly improves code readability if we try to adhere to the Rubocop/Ruby Style Guideline that suggests we should break up methods into internal "paragraphs" that make logical sense to be grouped together.

That can be a little nebulous but in this case some visual spacing really helps. So the main if/else block should have a space between the things before it to set it apart. Perhaps also set apart the array variable assignments from some of the handling below it. Things like that.

That's my final comment on this one unless @Raykyn55 has others, since ya'll are the crafting team.

asechrest avatar Aug 24 '22 21:08 asechrest

Thanks for the spacing. Looks much better. I lied about a final comment. @rpherbig notes that this is perhaps the 3rd or 4th place in code where we list the sigils. Shall we turn the sigil lists into a global?

asechrest avatar Aug 25 '22 15:08 asechrest

Thanks for the spacing. Looks much better. I lied about a final comment. @rpherbig notes that this is perhaps the 3rd or 4th place in code where we list the sigils. Shall we turn the sigil lists into a global?

I can take a look in doing that tonight and tomorrow night. If it makes sense, sure.

Dartellum avatar Aug 25 '22 15:08 Dartellum

Thanks for the spacing. Looks much better. I lied about a final comment. @rpherbig notes that this is perhaps the 3rd or 4th place in code where we list the sigils. Shall we turn the sigil lists into a global?

I can take a look in doing that tonight and tomorrow night. If it makes sense, sure.

A quick search on sigil in common-crafting only shows 9 matches all within my new method. Two of those are referencing sigilhunter.

Dartellum avatar Aug 25 '22 19:08 Dartellum

Thanks for the spacing. Looks much better. I lied about a final comment. @rpherbig notes that this is perhaps the 3rd or 4th place in code where we list the sigils. Shall we turn the sigil lists into a global?

I can take a look in doing that tonight and tomorrow night. If it makes sense, sure.

A quick search on sigil in common-crafting only shows 9 matches all within my new method. Two of those are referencing sigilhunter.

The primary and secondary list of sigils exist separately in attunement, sigilharvest, sigilrecorder, and now here.

asechrest avatar Aug 25 '22 20:08 asechrest

You mean like $primary_sigils = ["abolition", "congruence", "induction", "permutation", "rarefaction"] in common-crafting at the beginning?

Dartellum avatar Aug 25 '22 20:08 Dartellum

You mean like $primary_sigils = ["abolition", "congruence", "induction", "permutation", "rarefaction"] in common-crafting at the beginning?

Yes, that's my thought. Then we can utilize those globals directly here, in the 3 other scripts, and in any others.

asechrest avatar Aug 25 '22 20:08 asechrest

I will test.

Dartellum avatar Aug 25 '22 20:08 Dartellum

Those are not arrays in those scripts. I mean, not a bad idea to have them global since used in multiple scripts. So, I would need to take PRIMARY_SIGILS_PATTERN = /\b(?:abolition|congruence|induction|permutation|rarefaction) sigil\b/ and then make that into an array to check against the incoming.

The only real reason to keep the check if the sigil is in the list is for when Guild or new sigils are released that are not store bought. I would like to keep the script in the mode that some may not go on sale at the Society store in the future, just like secondaries were not for years.

Dartellum avatar Aug 25 '22 21:08 Dartellum

Those are not arrays in those scripts. I mean, not a bad idea to have them global since used in multiple scripts. So, I would need to take PRIMARY_SIGILS_PATTERN = /\b(?:abolition|congruence|induction|permutation|rarefaction) sigil\b/ and then make that into an array to check against the incoming.

The only real reason to keep the check if the sigil is in the list is for when Guild or new sigils are released that are not store bought. I would like to keep the script in the mode that some may not go on sale at the Society store in the future, just like secondaries were not for years.

I found a way to do it. Testing will begin!

Dartellum avatar Aug 25 '22 23:08 Dartellum

@Dartellum How's this looking? Thanks for adding the sigil globals. I'm showing this PR has conflicts to resolve.

asechrest avatar Sep 04 '22 13:09 asechrest

@Dartellum How's this looking? Thanks for adding the sigil globals. I'm showing this PR has conflicts to resolve.

Some updates need added. Working well for me and the few testing from Discord have not posted anything wrong.

Dartellum avatar Sep 04 '22 15:09 Dartellum

Resolved conflicts. Added a catch in case the same challenge occurs. Although, it did not in initial testing but rather safe than sorry.

Dartellum avatar Sep 04 '22 18:09 Dartellum