yulqen / datamaps

MOVED: Command line interface to yulqen/bcompiler-engine. Used by DfT to collect and clean data using Excel spreadsheets.
https://git.sr.ht/~yulqen/datamaps
MIT License
2 stars 2 forks source link

testing if datamaps v1.0.0 #4

Closed banillie closed 4 years ago

banillie commented 4 years ago

Hi Matt, started some testing of v1.0.0... did a master to template command... which raised some critical issues in relation to keys in the master not being present in the dm. Great functionality. However, it this shouldn't be seen as a critical issue that stops the command, because in the instance of master to template, there will (there should be the option to) be some key/values that the user doesn't want to move across to the template in order to prevent any formulas present in the template being over written.... we manage this process via tagging and filter keys in our masters dm document. e.g. if a key is relating to a value that is automatically updated in the template we mark it as auto and then filter it out when building the dm cvs file for master to template export. Could this check be relegated to below critical? cheers.

yulqen commented 4 years ago

Hi Will,

Exactly the kind of feedback needed so thanks. Will take a look tomorrow and update.

Pushed a couple of PRs your way tonight. Nothing major.

M

-- Matthew Lemon matt@matthewlemon.com

On Thu, 21 Nov 2019, at 5:35 PM, Will Grant wrote:

Hi Matt, started some testing of v1.0.0... did a master to template command... which raised some critical issues in relation to keys in the master not being present in the dm. Great functionality. However, it this shouldn't be seen as a critical issue that stops the command, because in the instance of master to template, there will (there should be the option to) be some key/values that the user doesn't want to move across to the template in order to prevent any formulas present in the template being over written.... we manage this process via tagging and filter keys in our masters dm document. e.g. if a key is relating to a value that is automatically updated in the template we mark it as auto and then filter it out when building the dm cvs file for master to template export. Could this check be relegated to below critical? cheers.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hammerheadlemon/datamaps/issues/4?email_source=notifications&email_token=AAB4ISJUHZPLJWTMRWYDNILQU3BEZA5CNFSM4JQFZPZ2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H3FOBEQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB4ISMLI7MEECLYS6GZEWLQU3BEZANCNFSM4JQFZPZQ.

banillie commented 4 years ago

Great cheers Matt. Yes starting to do more testing and prep for Q3 so should hopefully throw up some useful stuff.

Thanks for the PR will take look.

yulqen commented 4 years ago

Just to clarify, you said "keys in the master not being present in the dm". Do you actually mean the other way round - there is a key in the datamap which doesn't appear in the master? Only in the latter case does this CRITICAL error currently get raised as far as I can tell (https://github.com/hammerheadlemon/bcompiler-engine/blob/3d05209777ed9a4d63adf5c71e124a4e1e5bc995/engine/use_cases/output.py#L71)

If I remember correctly, the thinking was that the user manages the datamap, using filters and hiding rows, etc, to only make available those values in the datamap that should be exported (as you say in your issue). As long as he/she doesn't leave an entry in the datamap which does not appear in the master, the export should take place.

Are you saying that you would prefer it if datamaps skipped entries it finds in the datamap if a corresponding value isn't present in the master to be exported? I'm slightly confused in which case because I thought that was the purpose of filtering in the datamap. It would be dangerous to do it any other way as it is the datamap that should be the single version of the truth, not a master file. I'm obviously getting the wrong end of the stick here - maybe a chat would solve at some point!

Otherwise, if you could you encrypt and send me the files you are using to trigger this I could probably work it out...

banillie commented 4 years ago

The testing I did seemed to raise an error when the key was present in the DM, but not the master. Not the other way around.

I agree with what you say in relation to user requirements. It should raise a critical error if key present in DM but not in master.

I’ll try some further testing to double check. Will prob also send over a set of test docs, so you can try and replicate.

Cheers, Will.

yulqen commented 4 years ago

Ok. So if the key is in the datamap and not in the master, then the CRITICAL error is correct. The export cannot continue if you are asking it (via the datamap) to find a value in the master that isn't there - we should assume at all times that the datamap is correct. Hence your well thought-out facility to hide rows in the datamap. Hope this is right.

Happy to chat this weekend.

banillie commented 4 years ago

Sorry in the master and not in the DM. That’s what I believe is raising the error. I can test again tonight and confirm.

yulqen commented 4 years ago

Ok, let me know dude.

banillie commented 4 years ago

Hi Matt, ran the test again and was not able to replicate the error message and I got datamaps to work so apologies for false alarm!

However, when checking the templates that were created, data/information is being placed into the wrong cells in the templates. I’ve checked the DM to see whether the wrong key cell reference was provided but it looks right. So it might be that datamaps is not transferring properly? Sorry about this. Hopefully it’s me being stupid again!

I’ve sent the documents to your MR Lemon account so you can do testing also.

Thanks, Will.

yulqen commented 4 years ago

Ok Will - thanks. Will take a look!

On 23-11-2019, Will Grant wrote:

Hi Matt, ran the test again and was not able to replicate the error message and I got datamaps to work so apologies for false alarm!

However, when checking the templates that were created, data/information is being placed into the wrong cells in the templates. I’ve checked the DM to see whether the wrong key cell reference was provided but it looks right. So it might be that datamaps is not transferring properly? Sorry about this. Hopefully it’s me being stupid again!

I’ve sent the documents to your MR Lemon account so you can do testing also.

Thanks, Will.

-- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: https://github.com/hammerheadlemon/datamaps/issues/4#issuecomment-557839086

--

Matthew Lemon Email: matt@matthewlemon.com

yulqen commented 4 years ago

Oh dear - the current implementation of export relies on col A of the datamap being the same as col A in ther master, which is obviously not fit for purpose at all. Clearly you need to be able to adapt the datamap file to your needs (by hiding/omitting rows, as discussed in the comments above), and rely on the key names matching for a value to be exported.

What's your timescale to get this fixed?? This is obviously crucial and I will prioritise the fix.

banillie commented 4 years ago

Not massively urgent Matt. We have until 13 Dec for our commission. Although it would be very handy to be able to run tests to ensure the template is correct earlier than that.

Could we revert to an earlier version in the meantime? That would be useful.

Cheers. As always thanks for everything on this.

Will.

banillie commented 4 years ago

Hi Matt, is it possible to revert to an earlier version of datamaps, while this is being fixed? Just wondering if possible way to proceed. Cheers.

yulqen commented 4 years ago

Fix included in datamaps 1.0.1 - can you please try using pip-based installation first before I create the .exe file?

Set up a test virtualenv using Command Prompt

* cd C:\Users\standalone\Desktop
* python -m venv tmp-venv [or 'python3' here]
* tmp-venv\Scripts\activate.bat

Install datamaps

* pip install datamaps

Test whether bcompiler-engine has also been updated (pip list - both datamaps and bcompiler-engine should be v1.0.1). If not, pip install bcompiler-engine.

Run the test

* datamaps export master C:\PATH\TO\master_commission_test.xlsx`

If all goes ok, you can delete the Desktop\tmp-venv directory to remove the test virtualenv.

The export code in bcompiler-engine is the least tested and robust at this point unfortunately, so your continued testing/patience here is absolutely invaluable! I hope the above goes without a hitch. Report back with issues. If you're happy, I will release the exe on the site.

banillie commented 4 years ago

Hi Matt, I followed the instructions above and ran the export command. I got a the following

ModuleNotFoundError: No module named 'dataclasses'

cheers.

yulqen commented 4 years ago

Ok - this is the python 3.6 thing again. Scrap the idea of installing using the above - I'm not going to ask you to install Python 3.7 just now after putting you through that last time.

Just download the new dm.exe from https://datamaps.twentyfoursoftware.com/ and use that to test please (it should be v1.0.1). Sorry Will - I need to trap this problem about what Python version is being used by pip. I will also remove the use of dataclasses in bcompiler-engine because this is a 3.7 and above feature. None of this matters for users who download the exe because they get Python 3.7 bundled with it.

Let me know how you get on.

banillie commented 4 years ago

Have run and test and looks all good Matt. Data seems to be transferring nicely. 🐍 👍 💯

Will do some more testing over the coming days.

yulqen commented 4 years ago

Magic. Closing this now. Please continue to open issues as necessary!