Price Manager issues

[LordHelmchen]

From my opinion, the template file should contain the minimal information about properties/functions and their usages (e.g. examples), and the How-to should be a reference where developers can find/share more completed examples (e.g. different scenarios like the foil/regular prices on same page).

Agreed. I will try to keep the template self-documenting, but refrain from adding multiple conflicting example implementations. In return, the wiki can hold examples, including but not limited to, the ones already present in the template.

Please do point out anything that you're unsure about, so I can verify it.

You can find some "TODO" in the how-to for points that i'm not sure about it or I didn't know how to describe in the good way

Sounds good to me.I'll keep on checking for them and resolving them, and you keep on adding them

For the structure, I tried to make it like a wizard where each required properties are coming first and afterwards optional properties and LHpi properties. At the end, the functions to define.
In my case, I "reformatted" your sitescript template in that order.

If the how-tov and template file have the same structure, developpers can read the how-to and complete their sitescript at the same time. They know what the property/function does and how-to fill it.

I agree on the howto being structured in order of implementation priority, and will leave the ordering to you (as I'm probably biased).
I disagree with reordering the template:

• Boolean control options are placed in front. Some are potentially needed to be already define for the rest of the sitescript, including function definitions, to execute properly. They also are the only thing the end user should need to edit once the template becomes a released sitescript, so having them accessible at the front makes sense.
• global configuration elements (like libver,dataver,scriptname) and the global container tables (LHpi and site) need to be defined for the rest of the script to execute properly.
• site configuration elements (regex,resultregex,currency,encoding) could be moved down and grouped with the tables, if that seems more convenient or logical.
• running the sitescript with undefined functions will result in LHpi (intentionally and conrolled) throwing an error, while running with empty tables will usually just find nothing to import, so functions were placed above tables due to priority.

Of course, this is open source, and me disagreeing does not mean I'm not grateful for the ideas nor open to further disussion

Should the paragraphs from the Hints for developers section stay seperate or be moved over to the howto?

The hint are welcome in the how-to, in the appropriate section.
"multiple copies of the same script" could be added in the "(semi-optional) Name of sitescript".
"procedure of funtions call" could be added in separate section (e.g. annexes ?).
"Dropping card" could be added to the appropriate function definition (ParseHtmlDate, BuildCardData, ...) depending the usage developpers do.
"non trivial tables" could be added as a general tips in the properties introduction.

Will do. I rather like seperating Development (of the library) and Developing (sitescripts for the library) in this way.

Shall I add information about the return values that LHpi expects from the sitescript functions, or are the template comments enough for this?

You're welcome to add these infos in the how-to.

Will do (eventually) Feel free to start it, adding TODO markers where needed.

Last (for this post, anyway ) remark about the wiki: I'm not documenting changes I make to the wiki in this thread. In this phase of rapid changes to the documentation, I regularly check the wiki's history function and would propose you do the same.

-----------------------
Apart of the wiki page, I have another question about LHpi.

1. Library updade
   ...

As I'm working towards libver 2.8, I already expected the need for and planned to add "updating sitescripts" sections to the howto, where I will try to document all needed changes.
Until that exists, you can either continue using the library version you're most comfortable with (sitescripts were made to be able to specify and load their preferred library version for this reason), or you may need to use some sort of

diff | Open

are you familiar with this, or does the howto need a section on diff tools as a general hint?

tool to see what is changed and adapt your sitescript. The version history should already give you some small hints on what needs to be adapted, too. Copy/paste from old into new might also work, and potentially prevent the luadoc comments from becoming out of sync (which is a pain to remember updating for all four of my sitescripts each time ).

In any case, it is probably a very good idea to use ImportPrice from the template version that matches the library version, and only incorporate changes you might have made to it into the the new function definition.

I apologise for the inconvenience this causes.

I will only maintain the template for the latest library version, but library and data versions will stay up in their respective sections as long as there is any current sitescript that depends on that version (and even if not, old versions can still be downloaded via the version history).

Different url infix for same set

In the last days, the Belgian website did an important update, causing the failure of the my sitescipt.

I just love when that happens. See the first two entries in trader-onlineDE version history for my favorite moment so far

...
Do you think it's the correct way ? Or you see another one ?

2.8 will bring an extended site.frucs format, that better deals with importfoil="n" or "o" cases:

new site.frucs structure | Open

Code: Select all

--[[- table of available rarities.
 can contain url infixes for use in site.BuildUrl

 @field [parent=#site] #table frucs   rarity array { #number = #string } ]]
--Thre three sample fruc tables below should cover most cases
site.frucs = {
   [1]= { id=1, name="Foil"   , isfoil=true , isnonfoil=false, url="foil" },
   [2]= { id=2, name="nonFoil"   , isfoil=false, isnonfoil=true , url="regular" },
}
--site.frucs = {
--   [1]= { id=1, name="Foil"   , isfoil=true , isnonfoil=false, url="foil" },
--   [2]= { id=2, name="Rare"   , isfoil=false, isnonfoil=true , url="rare" },
--   [3]= { id=3, name="Uncommon", isfoil=false, isnonfoil=true , url="uncommon" },
--   [4]= { id=4, name="Common"   , isfoil=false, isnonfoil=true , url="common" },
--   [5]= { id=5, name="Purple"   , isfoil=false, isnonfoil=true , url="timeshifted" },
--}
--site.frucs = {
--   [1]= { id=1, name="nonfoil+Foil"   , isfoil=true , isnonfoil=true, url="" },
--}


If the ID of foil and nonfoil pages are somehow related, you could get away with soing some computation between site.sets[sid].url and site.frucs[fid].url fields in your site.BuildUrl.
Should I hurry and release 2.8 (and continue going to 2.9), so you can use this already?

2.7 -> 2.8 changes done so far | Open

Code: Select all

added savepath writable check to SAVECSV==true in DoImport
don't assume fruc[1] is foil and all other frucs are nonfoil
   site.fruc: add booleans to have each fruc identify itself as foil and/or nonfoil
   site.BuildUrl: infix changed from site.frucs[frucid] to site.frucs[frucid].url
   ProcessUserParams: disable unwanted frucs in site.sets at runtime, similar to how unwanted langs are already handled
   ListSources: removed check for url.foilonly, ProcessUserParams should have taken care of it.
   BuildCardData: removed @param #boolean urlfoil, as it was not reliable
                  (site.BuildUrl can return the same url twice, with different url.foilonly;
                   only the first of the duplicate urls is kept).
                   If you need to query url.foilonly, do it in site.ParseHtmlData.
   BuildCardData: make sure userParams are honoured, even when preset data is present, by setting unwanted prices nil before returning the card

plans for either 2.8 or 2.9 | Open

Code: Select all

add CHECKSTRICT option below CHECKEXPECTED to toggle notallgood triggering on drop,namereplace,foiltweak; otherwise, CHECKEXPECTED will only care about prices set and failed.

to anticipate sites that return "*CARDNAME*REGPRICE*FOILPRICE* instead of "*CARDNAME*PRICE*FOILSTATUS*"
 have site.ParseHtml return a collection of cards, similar to site.BuildUrl

Otherwise ... I think you found the best solution. site.sets[sid].url is not used in the libray, so its safe to define it however you need it to get BuildUrl do what is right.

(Still a good example to add in the how-to )

feel free to run wild with it

[anthonybe]

First of all, sorry for my late response.

I changed the order my LUA script according to the order of the wiki and did not notice any problem
I will send you the script by PM, so you can see how is it.

Last (for this post, anyway ) remark about the wiki: I'm not documenting changes I make to the wiki in this thread. In this phase of rapid changes to the documentation, I regularly check the wiki's history function and would propose you do the same.

Will do

As I'm working towards libver 2.8, I already expected the need for and planned to add "updating sitescripts" sections to the howto, where I will try to document all needed changes.
Until that exists, you can either continue using the library version you're most comfortable with (sitescripts were made to be able to specify and load their preferred library version for this reason), or you may need to use some sort of tool to see what is changed and adapt your sitescript. The version history should already give you some small hints on what needs to be adapted, too. Copy/paste from old into new might also work, and potentially prevent the luadoc comments from becoming out of sync (which is a pain to remember updating for all four of my sitescripts each time ).

In any case, it is probably a very good idea to use ImportPrice from the template version that matches the library version, and only incorporate changes you might have made to it into the the new function definition.

I use Kdiff3 for thins kind of comparison.
No problem with that. I will re-create my script with your latest library and do copy/paste.
Once my script is done, I will keep the library version and do the update only if required.

Concerning 2.8, no urgence. I will do the solution I proposed.

Concerning the Library, I have an idea for improvement.
For now, the template can define site.variants and if it does, it need to define all possible variants (already defined in LHpi) even if template add only one variants.
It would be great if LHpi could merge the default variants (defined by LHpi) with variants defined by the template, with precedence over template variants.
In that case, template can define only ONE variant it found usefull and do not copy/paste ALL variants from LHpi, which is a pain to maintain (LHpi <> template).

[LordHelmchen]

First of all, sorry for my late response.

I was slightly worried that the latest flurry of changes discouraged you. Glad to see you're still with me.

I changed the order my LUA script according to the order of the wiki and did not notice any problem
I will send you the script by PM, so you can see how is it.

I'll check it out and try to come up with helpful remarks

I use Kdiff3 for thins kind of comparison.
No problem with that. I will re-create my script with your latest library and do copy/paste.
Once my script is done, I will keep the library version and do the update only if required.

Concerning 2.8, no urgence. I will do the solution I proposed.

2.8 is released already, with significant improvement to fruc handling and also some bugfixes.

lib 2.8 changelog | Open

Code: Select all

DoImport: added savepath writable check to SAVECSV=true
    don't assume fruc[1] is foil and all other frucs are nonfoil

        site.fruc: add booleans to have each fruc identify itself as foil and/or nonfoil
        site.BuildUrl: infix changed from site.frucs[frucid] to site.frucs[frucid].url
        ProcessUserParams: disable unwanted frucs in site.sets at runtime, similar to how unwanted langs are already handled
        ListSources: removed check for url.foilonly, ProcessUserParams should have taken care of it.
        BuildCardData: removed @param #boolean urlfoil

    BuildCardData: make sure userParams are honoured, even when preset data is present, by setting unwanted prices nil before returning the card
    BuildCardData: BCDplugins should know everything BCD knows, so we're passing them importfoil, importlangs
    GetSourceData: now expects site.ParseHtmlData to return card(s) wrapped in a container table
    new STRICTCHECKEXPECTED option
    Toutf8: new optional parameter enc
    string.format'ed all nontrivial Log(string)s
    read static language fields from LHpi.Data.languages
    shortened site.langs
    hotfix: two bugs fixed (thanks to Bloodnut for reporting them)

There's an updated template that incorporates those changes.
Also, 2.9 is almost ready, with the third bug found by Bloodnut fixed (and should put an end to to this phase of rapid changes).

lib 2.9 changes | Open

Code: Select all

(all changes so far are transparent to sitescripts)
   fixed and improved MergeCardrows (was broken if importfoil=="N" or "O" - reported by Bloodnut)
   got rid of most global variables
      curhtmlnum      local in MainImportCycle
      persetcount    local in MainImportCycle
                  BuildCardData also returns #boolean namereplaced, #boolean foiltweaked
                  SetPrice also returns #table psetcount, #table failcount
      totalcount,setcountdiffers      local in DoImport
                  MainImportCycle returns #table totalcount, #table setcountdiffers
   misc. small improvements to code and/or comments
   fixed loading of data file from deprecated "Prices" location
   fixed SAVETABLE folder writable check
   fixed logging resultregex finds

lib 2 changes | Open

Code: Select all

(all changes transparent to sitescripts)
   minor improvements to some special sets' variant
   misc. small improvements to code and/or comments
   added 636:Salvat 2011

template 2.9.2 changes | Open

Code: Select all

too much simplification prevented possible functionality: reintroduced site.langs[langid].id field for reverse lookup
   split script version from scriptname to ease possible future change to versioning scheme
   misc. small improvements to example code and/or comments

You may want to prepare your script to be 2.9.2.x ... I apologise for putting you in a moving-goalpost-situation.

Concerning the middle point of sitescript changes: I have now moved LHpi development to a local git repository, so that should take care of having multiple (dev) versions and identifying which one to use within Magic Album for me. Thus, I think it's better (from an enduser perspective) to remove the version numbers from the sitescripts' filenames. They still need to be defined within the scripts, both for lib/data loading and to identify the version in the logfile, and lib,data and template will still have them in the filename.

some more rambling about my reasons | Open

The advantage (for the user) is that the .csv files that MA stores the imported prices in will not accumulate (leading to a cluttered "\Prices") and old prices that are removed from the websites are still available (as the import will not overwrite existing prices with 0). I'm still on the fence if the logfiles chould carry version numbers or not (clutter vs. easy identification and multiple versions in paralell). Additionally, having the version numbers only inside the script removes redundancy and the risk of going out of sync.
The disadvantage is that I need a few more click to prepare a release to add version numbers to the zipfile, and that an unsuspecting user could accidentally overwrite old files that he wants to keep.

Of course, this only really affects my sitescripts, so you're free to adapt any versioning and file naming schmeme that fits your needs. The only change is the assurance that the

Code: Select all

scriptname = "LHpi.sitescriptTemplate-v" .. libver .. "." .. dataver .. "." .. scriptver .. ".lua"


is not as vital as is seemed to be

Concerning the Library, I have an idea for improvement.
For now, the template can define site.variants and if it does, it need to define all possible variants (already defined in LHpi) even if template add only one variants.
It would be great if LHpi could merge the default variants (defined by LHpi) with variants defined by the template, with precedence over template LHpi.data variants.
In that case, template can define only ONE variant it found usefull and do not copy/paste ALL variants from LHpi, which is a pain to maintain (LHpi <> template).

Funny thing you mention that. I was stumbling over that just now while testrunning the new 2.9.2.1 sitescripts (Do I really want to duplicate all basic lands variants for the sake of two soldier tokens?) ...
I initially decided against it, because I wanted to avoid non-obvious states, so it's either default or sitescript variants table. Further, once you have a variant table for a set that works with your queried site, no further changes should be necessary to that set's variants (unless the site (or the set ) changes). On the other hand, I agree that reducing redundancy/code duplication is the whole point of a library, so I'm open to suggestions on how this should behave. Are we sure that there is no caveat in having unused/unwanted variant definitions from the default tables hanging around after merging? If the same cardname is defined in both LHpi.Data and sitescript, sitescript should take precedence? do we prefer the additional loops and runtime cost the merging needs to the need to replicate the default table? (anything said about variants applies to and will be implemented similarly to foiltweak)

[anthonybe]

Concerning my sitescript and version 2.9, I will wait you release the new library before rewriting my sitescript. For now, I use version 2.7 and the sitescript is broken due to Belgian website refactoring.
I did not have time to fix the script until now. So waiting the release 2.9 is a good starting point to start again

Concerning the variants, does it really cost to merge LHpi and sitescript at runtime ?
I don't know performance of LUA script but it should not cost too much time to do the merge. Does it?
Concerning duplication of variants definition, do we need to do it or not ? I would say that LHpi should take care of default variants and if the sitescript does not have expected results, it will add needed variants over LHpi. So, the variants priority would be: First use sitescript variants; if sitescript does not have variants for card name, use the one from LHpi if it exists.

[LordHelmchen]

So waiting the release 2.9 is a good starting point to start again

2.9 is released, and library development should have stabilized again. hope it works for you. Would you be so kind and delete and/or incorporate into the main text the sections on updating to new sitescript version and the version specific notes, once you no longer need them?

Concerning the variants,

Probably not that much of a cost, especially considering how much looping LHpi already does (probably half of them unnecessary or only for sanity checks ).
handling of default variant/foiltweak tables is on the list of future improvements, but I think it can wait (probably until BNG NYX triggers another flurry of development). Merging custom (site) and default (data) tables instead of only using data if site is undefined means I will also need to add a trigger to skip merging and hard-override the default tables. (LHpi.mtgmintcard has an example where this will be necessarry once merging is implemented.) Without this trigger, we would need to explicitely nil each unwanted entry seperately, so the maintenance problem would only be reversed.

[LordHelmchen]

turns out the runtime cost is negligible

performance tests | Open

dummy.loadscript("LHpi.mtgmintcard.lua",path,savepath)--2.9.2.9
OFFLINE = true
SAVELOG = true
local fakeimportsets = coresets
ImportPrice( fakeimportfoil, fakeimportlangs, fakeimportsets )

minimal
run 1: 6.476 seconds
run 2: 6.88 seconds
run 3: 5.499 seconds
run 4: 5.132 seconds
run 5: 5.173 seconds
run 6: 5.185 seconds
run 7: 5.154 seconds
run 8: 5.286 seconds
run 9: 5.039 seconds
run 10: 5.07 seconds

full log (VERBOSE,LOGDROPS,LOGNAMEREPLACE,LOGFOILTWEAK,CHECKEXPECTED,STRICTEXPECTED)
run 1: 7.16 seconds
run 2: 6.98 seconds
run 3: 7.07 seconds
run 4: 7.07 seconds
run 5: 7.13 seconds
run 6: 7.15 seconds
run 7: 7.05 seconds
run 8: 7.01 seconds
run 9: 7.05 seconds
run 10: 7.05 seconds

full DEBUG (DEBUG,DEBUGFOUND)
run 1: 94.3 seconds
run 2: 93.3 seconds
run 3: 91.5 seconds
run 4: 92.5 seconds
run 5: 91.7 seconds
run 6: 93.5 seconds
run 7: 93.6 seconds
run 8: 94.4 seconds
run 9: 91.3 seconds
run 10: 92.9 seconds

new variant,foiltweak merge, full log
run 1: 7 seconds
run 2: 6.84 seconds
run 3: 7.37 seconds
run 4: 6.98 seconds
run 5: 6.98 seconds
run 6: 6.88 seconds
run 7: 7.05 seconds
run 8: 6.91 seconds
run 9: 6.94 seconds
run 10: 7.36 seconds

Feature request implemented.

[anthonybe]

Thank you very much for the fast implementation/perf tests.

I will download the 2.10 version and rewrite sitescript from the template.
Once working, I will reorder the sitescript to be compliant with wiki order and see if it's still working

I will keep you informed about that test.