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

Adding fiat_ticker to manual.py transactions #134

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

topherbuckley
Copy link
Contributor

As per discussion here

I'll add unit tests and such once I figure out the build flow:

Reading through the dev workflow it lists bin/dali_us in LOG_LEVEL=DEBUG bin/dali_us -s -o output/ config/test_config.ini I don't see a bin directory in the root repo dir, so maybe this is referring to the .env/bin dir? If so I'm not quite sure how my changes in src are "compiled" I know thats the wrong word here, but I've never done python packaging. In other words how do I push my changes in src to be used in the call to dali_us? Or should I just be invoking dali_entry() from a python interpreter?

@eprbell
Copy link
Owner

eprbell commented Apr 3, 2023

As per discussion here

I'll add unit tests and such once I figure out the build flow:

Reading through the dev workflow it lists bin/dali_us in LOG_LEVEL=DEBUG bin/dali_us -s -o output/ config/test_config.ini I don't see a bin directory in the root repo dir, so maybe this is referring to the .env/bin dir? If so I'm not quite sure how my changes in src are "compiled" I know thats the wrong word here, but I've never done python packaging. In other words how do I push my changes in src to be used in the call to dali_us? Or should I just be invoking dali_entry() from a python interpreter?

Ah, yes, bin/ shouldn't be there: I edited it out. If you follow the setup instructions, you can modify the source locally and then just call dali_us directly. This will call the code in your src directory and will reflect your changes. Hope this helps.

@topherbuckley
Copy link
Contributor Author

Ah, yes, bin/ shouldn't be there: I edited it out. If you follow the setup instructions, you can modify the source locally and then just call dali_us directly. This will call the code in your src directory and will reflect your changes. Hope this helps.

Thanks! So wouldn't that then reference <rp2_directory>/.venv/bin/dali_us? I guess it is not clear to me how this would be then backreferencing the src in the dali-rp2 directory, but I suppose it is some magic within installing the extras with the -e flag in .venv/bin/pip3 install -e '.[dev]' right? I'll read up on that to understand that.

I'll work on getting this PR up to date in next few days. Thanks for your help!

@eprbell
Copy link
Owner

eprbell commented Apr 4, 2023

Yep, .venv/bin/pip3 install -e '.[dev]' is the command that does the magic :) It installs DaLI in development mode, which means that it gets installed with a symlink to the src directory. So any changes to the source code get immediately picked up when you run DaLI.

@topherbuckley
Copy link
Contributor Author

topherbuckley commented Apr 9, 2023

Ok, managed to get the workflow working and figured out my mistake in the previous error pretty easily thereafter.

Hold off on starting the workflows though, as I noticed I have some issues with the unit tests locally.

@topherbuckley
Copy link
Contributor Author

topherbuckley commented Apr 9, 2023

Where are the local tests supposed to be run from? I get failures if running from the dali-rp2 root, but all pass if running from the rp2 root. Is this the intended workflow?

@macanudo527
Copy link
Collaborator

macanudo527 commented Apr 9, 2023

Where are the local tests supposed to be run from? I get failures if running from the dali-rp2 root, but all pass if running from the rp2 root. Is this the intended workflow?

They should run just fine in dali-rp2 root.

Keep in mind that you have to activate the virtualenv environment:
. .venv/bin/activate

And that environment is only for dali-rp2. So, if you activated the environment in the rp2 root and then moved to dali-rp2 without deactivating the virtualenv, the tests won't run. You will have to deactivate then activate in the new directory.

What errors are you getting specifically?

@eprbell
Copy link
Owner

eprbell commented Apr 9, 2023

Correct and I'll also add that RP2 is a dependency for DaLI, so when you install DaLI, RP2 gets automatically installed too. This means that there is no need to refer to the RP2 directory if you're doing DaLI development.

@eprbell
Copy link
Owner

eprbell commented Apr 9, 2023

I intended to add a reply but ended up mistakenly modifying @macanudo527's last comment instead (the one about activate). I did my best to restore it in its original form, based on the email I had received: if I missed anything please add it back. Sorry about that.

@topherbuckley
Copy link
Contributor Author

Thanks for the info! I was using the venv from the rp2 root, so that sounds like the misunderstanding. I'm not on the same machine at the moment, but I recall it being something to do with a missing file used by one of the unit tests. The way the paths were specified prompted me try from the rp2 root, after which the unit tests all passed.

After hearing your feedback, I think I can make sense of it when I get back to that machine. Sorry again, I'm sure my confusion arises from a lack of knowledge surrounding the workings of pip install -e '[dev]'. I'll have a look at this again tonight or tomorrow and follow up thereafter.

@macanudo527
Copy link
Collaborator

Thanks for the info! I was using the venv from the rp2 root, so that sounds like the misunderstanding. I'm not on the same machine at the moment, but I recall it being something to do with a missing file used by one of the unit tests. The way the paths were specified prompted me try from the rp2 root, after which the unit tests all passed.

I think I did basically the same thing myself and it showed up like that because the ROOT is different, so that is probably what happened.

@topherbuckley
Copy link
Contributor Author

Maybe I'm misunderstanding something, but is the README.dev.md incorrect when it specifies the <rp2_directory> then?

cd <rp2_directory>
virtualenv -p python3 .venv
. .venv/bin/activate
.venv/bin/pip3 install -e '.[dev]'

Should this not be

cd <dali-rp2_root_directory>
virtualenv -p python3 .venv
. .venv/bin/activate
.venv/bin/pip3 install -e '.[dev]'

@eprbell
Copy link
Owner

eprbell commented Apr 15, 2023

Maybe I'm misunderstanding something, but is the README.dev.md incorrect when it specifies the <rp2_directory> then?

cd <rp2_directory>
virtualenv -p python3 .venv
. .venv/bin/activate
.venv/bin/pip3 install -e '.[dev]'

Should this not be

cd <dali-rp2_root_directory>
virtualenv -p python3 .venv
. .venv/bin/activate
.venv/bin/pip3 install -e '.[dev]'

Ouch, copy/paste error: thanks for reporting! I just fixed it.

@topherbuckley
Copy link
Contributor Author

topherbuckley commented Apr 15, 2023

Rerunning the local pytests I see the cause of at least one error, is the absence of this fiat-ticker column in the test input csv files used by the config/test_config.ini i.e.:

  • input/test_manual_in.csv
  • input/test_manual_intra.csv

I refactored the value being used to check column length and such from __XX_NOTES_INDEX to __XX_LEN as opposed to using __XX_FIAT_TICKER as I thought it would be cleaner and easier to add additional columns by changing just one global value rather than at all logical checks in the code. Let me know if you disagree with this and I can change it back to using __XX_FIAT_TICKER.

Even with this though, all users would be forced to add the fiat_ticker column to their data, or worse, if they have unused columns for their own purposes, these will get used unintentionally as the fiat_ticker values. So rather than just adding this column to the

  • input/test_manual_in.csv
  • input/test_manual_intra.csv

files, I think I need to:

The best way to fix this would be to add field index qualifiers to the manual plugin (similar to what RP2 does with the in_header, out_header and intra_header sections of its configuration file.

As mentioned here.

Will continue working on this later. Thanks for your patience!

@topherbuckley
Copy link
Contributor Author

I just force pushed a larger overhaul of this, but its still a wip. I had some questions that I need to reference specific lines of code over in this discussion.

src/dali/dali_main.py Outdated Show resolved Hide resolved
@eprbell
Copy link
Owner

eprbell commented May 27, 2023

I just force pushed a larger overhaul of this, but its still a wip. I had some questions that I need to reference specific lines of code over in this discussion.

I replied inline: let me know if you have more questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants