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

Should we reorganize the root directory? #23

Closed
Conr86 opened this issue Nov 6, 2019 · 26 comments · Fixed by #26
Closed

Should we reorganize the root directory? #23

Conr86 opened this issue Nov 6, 2019 · 26 comments · Fixed by #26
Assignees
Milestone

Comments

@Conr86
Copy link
Member

Conr86 commented Nov 6, 2019

We can consider making a few changes before the v1.0 release such as:

  • rename manager.py to main.py to better reflect it's importance
  • move other scripts into a subdirectory (src/ perhaps?)
  • move sensor_wrapper.py into sensors/
@Conr86 Conr86 added this to the v1.0 milestone Nov 6, 2019
@WilliamsJack
Copy link
Member

I agree with these changes, and it would be better done sooner rather than later. src/ doesn't seem like the best name for a scripts directory though. Theoretically, everything should go in src/, (src/ being project source code) and we add additional directories or files in the root directory for documentation and tests.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Perhaps we should move all Python stuff to src/ or perhaps sights/ (I think I'd prefer sights, as many Python packages are organised like that, such as supervisor_sights_config)

.
├── Arduino/
│   ├── *
├── configs/
│   ├── *
├── install.sh
├── LICENSE
├── main.py
├── README.md
├── requirements.txt
└── src/	(or sights?)
	├── control_receiver.py
	├── sensor_stream.py
	├── sensor_wrapper.py
	├── servo_party.py
	├── websocket_process.py
	├── utils/
	│   └── *
	└── sensors/
  		└── *

What's your opinion?

@WilliamsJack
Copy link
Member

WilliamsJack commented Nov 7, 2019

I agree, sights is a better name for that subdirectory containing the Python files.

The whole file tree you created above could itself be a subdirectory called src. Then, the root directory can have a cleaner structure containing other files and directories such as the installer, GitHub files like the readme and license as well as folders for docs and tests if we ever write them:

.
├── docs/
│   └── *
├── .gitignore
│   install.sh
├── LICENSE
├── README.md
├── src/
│   ├── Arduino/
│   │   └── *
│   ├── configs/
│   │   └── *
│   ├── main.py
│   ├── requirements.txt
│   └── src/	(or sights?)
│       ├── control_receiver.py
│ 	├── sensor_stream.py
│ 	├── sensor_wrapper.py
│       ├── servo_party.py
│ 	├── websocket_process.py
│ 	├── utils/
│ 	│   └── *
│ 	└── sensors/
│           └── *
└── tests/
    └── *

This tree was handwritten, there might be inconsistencies

This seems to be the structure used by a lot of popular repositories:
https://github.com/openai/gpt-2
https://github.com/kubernetes/kubernetes
https://github.com/apache/spark
https://github.com/Microsoft/vscode
https://github.com/NixOS/nixpkgs
To name a few from https://hackernoon.com/githubs-top-100-most-valuable-repositories-out-of-96-million-bb48caa9eb0b

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

I think I'd prefer the configs in the top level since they're not really source code and it makes it a little clearer that the user is allowed to edit them. Also the apache and motion, etc. configs don't really belong in a src directory.

This would be the final structure:

.
├── configs/
│   └── *
├── docs/
│   └── *
├── .gitignore
├── install.sh
├── LICENSE
├── README.md
├── src/
│   ├── Arduino/
│   │   └── *
│   ├── main.py
│   ├── requirements.txt
│   ├── control_receiver.py
│ 	├── sensor_stream.py
│ 	├── sensor_wrapper.py
│   ├── servo_party.py
│ 	├── websocket_process.py
│ 	├── utils/
│ 	│   └── *
│ 	└── sensors/
│           └── *
└── tests/
    └── *

Or collapsed to show the "cleanliness" of the TLD:

.
├── configs/
├── docs/
├── .gitignore
├── install.sh
├── LICENSE
├── README.md
├── src/
└── tests/

@Conr86 Conr86 self-assigned this Nov 7, 2019
@WilliamsJack
Copy link
Member

Moving the configs (the robot configs specifically) up to the top level is a great idea, however, I'd argue that we should keep apache and motion configs away from the user since they are to be used by the SIGHTS installer, and aren't meant to be edited by user.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

How about we move all non-SIGHTS configs to a etc/ or similar directory in the root directory?

@WilliamsJack
Copy link
Member

I think it's still too exposed to the user, however we do need to consider users who are using the manual install instructions. I think it's a good compromise.

The configs directory has a subdirectory internal that should remain in src. Currently it contains the minimal.json config used for booting to safe mode.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Actually I just realised that the motion configs (camera0.conf etc) need to be able to be edited by the user. The motion.conf file is currently only updatable by editing it in /etc/motion/ but we might want to change that to a symlink so the user can edit it from the SIGHTS directory to change framerate, resolution and disable other cameras if they're never going to be used. I still want these seperate from the SIGHTS configs though. So we could put motion configs in an etc directory or something. As for apache2 and supervisor configs, I'm not sure, we could make an install dir or something that contains files that are only used by the installer?

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Actually let's use the internal directory for that?

@WilliamsJack
Copy link
Member

Good point. Come to think of it I don't think internal is a great name on its own, it only worked as a subdirectory of configs. Perhaps we could have a configs directory on the top level for robot configs and also a configs directory within src for internal configs used by either SIGHTS or the SIGHTS installer.

@WilliamsJack
Copy link
Member

Your idea for motion config symlinks is good too, forgot to mention that.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Brilliant. Sounds like a plan. I'll make a branch and reorganise everything, without fixing dependencies initially, so we can see how it looks.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Should we rename internal to sights to make it clear that it's for SIGHTS itself? It's clear that it's internal since it'll be in the src/ subdirectory

@WilliamsJack
Copy link
Member

WilliamsJack commented Nov 7, 2019

For the config files? sights is too general since it's the name of the software... Maybe if it was inside a configs directory?

.
├── configs/              # User-generated robot configs
│   └── *
├── src/
│   ├─ configs/           # Configs hidden from the user
│   │   ├── sights/       # Used by SIGHTS under normal operation
│   │   │   └── backups/  # Backups generated by SIGHTS
│   │   └── installer/    # Used only by the installer
⋮    ⋮

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Yeah sorry that was what I meant. Your directory structure is basically what I have at the moment, except I have apache2 etc instead of installer

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

What do you think of https://github.com/SFXRescue/SIGHTSRobot/tree/restructure so far?

@WilliamsJack
Copy link
Member

That root directory looks so clean!

Is sensor_wrapper.py in src/ going to be moved to src/sensors/?

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Possibly not... Not sure to be honest. I think I'd prefer to have just sensor wrappers in src/sensors/. Now that the root dir is real clean, I'm less stressed about tidying up src/.

@WilliamsJack
Copy link
Member

Good call.

While I do have fond memories of the name servo_party.py, I don't think it accurately reflects that script anymore. How about motors.py to reflect the terminology used by the config?

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Holy moly that's a great idea, I never even thought of that. That's far more fitting.

Since supervisor and apache2 configs don't need to be updated by the user (but might need to be updated by the SIGHTS updater, hence symlinks) I want to leave them in src/configs/ but what about Motion? We could put it in configs/ I guess? or etc/motion?

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

Having motors.move() in the code looks so much neater than servo_party.move()

@WilliamsJack
Copy link
Member

On the topic of motion config symlinks, how about configs/motion/? Adding another directory etc/ seems unnecessary just for that.

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

I'm happy with using configs/motion/. Then it's clear that configs/ is for user editable configs, and src/configs/ is for internal use.

@WilliamsJack
Copy link
Member

WilliamsJack commented Nov 7, 2019

Just to check we're on the same page, you're talking about having the motion config files in src/configs/motion. The installer moves them to /etc/motion/ (the OS directory). The project's configs/motion/ directory contains symlinks to the OS directory /etc/motion/.

Have I got that right?

@Conr86
Copy link
Member Author

Conr86 commented Nov 7, 2019

I was just thinking of having motion configs in the root configs/ directory, and in the installer creates symlinks (in /etc/motion/) to that directory.

@WilliamsJack
Copy link
Member

Ahh, the reverse. Nice, that's probably a better solution. Not sure why I even thought the configs should be in src/configs/ when they're just going to be moved right out of that folder by the installer 😆

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

Successfully merging a pull request may close this issue.

2 participants