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

Consider restructuring the project modules (lib/gui/plugins) #4279

Closed
toxeus opened this issue Apr 19, 2018 · 8 comments
Closed

Consider restructuring the project modules (lib/gui/plugins) #4279

toxeus opened this issue Apr 19, 2018 · 8 comments

Comments

@toxeus
Copy link
Contributor

toxeus commented Apr 19, 2018

This is a minor issue but still worth reporting.

test_electrum_seed_2fa was added in this commit which imports plugins.trustedcoin which imports electrum and its submodules. As a consequence breaking changes in electrum or its submodules will not break the test until the electrum package is reinstalled. Even worse, a git pull might suddenly result in the test failing, assuming that the test and trustedcoin were changed.

@toxeus
Copy link
Contributor Author

toxeus commented Apr 25, 2018

@SomberNight in my personal projects I use the following project structure which isn't affected at all by the problems electrum currently faces.

repo
├── script
├── pkg1
├── pkg2
└── ...

The hacks in the main script are not needed and the unit tests use the newest code from the repo because Python puts the directory containing the input script on top of its module search path (consequently pytest needs to be executed from the projects root dir; not sure if this is already the case).

For the electrum project the structure would therefore look like the following (only listing relevant files and folders).

electrum
├── electrum.py
├── electrum
├── electrum_gui
└── electrum_plugins

@SomberNight SomberNight changed the title tests: test_electrum_seed_2fa doesn't test code from repo Consider restructuring the project modules (lib/gui/plugins) Apr 26, 2018
@SomberNight
Copy link
Member

@ecdsa @ysangkok @bauerj
probably of interest to all of you

@SomberNight
Copy link
Member

(context: #4286 (comment) )

@ysangkok
Copy link
Contributor

ysangkok commented Jul 1, 2018

@toxeus Try this: https://github.com/ysangkok/electrum/tree/file_layout_change

What do you think? I haven't used your suggested layout since it requires more hacks when there is no module that contains all relevant sources. This still needs to be tested on all platforms except Linux.

@toxeus
Copy link
Contributor Author

toxeus commented Jul 7, 2018

@ysangkok sorry for the late reply. I don't know how to review 265 commits efficiently.

screenshot_2018-07-07 ysangkok electrum 1

This branch seems to have different kinds of changes intermingled.

screenshot_2018-07-07 ysangkok electrum

@ysangkok
Copy link
Contributor

ysangkok commented Jul 7, 2018

It's just one commit on top of a now-rebased lightning branch. But after reading your other comment, I don't see any particular problems with your approach.

@toxeus
Copy link
Contributor Author

toxeus commented Jul 7, 2018

It's just one commit on top of a now-rebased lightning branch.

I see. Sorry for the misunderstanding then 😅

@ysangkok
Copy link
Contributor

Fixed in 097ac14 . Thanks for the report.

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

No branches or pull requests

3 participants