-
Notifications
You must be signed in to change notification settings - Fork 195
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
Implemented custom directory structure #269
base: master
Are you sure you want to change the base?
Implemented custom directory structure #269
Conversation
This PR implements a custom directory structure for fbs. Note: the previous directory structure is still used by default. Implementation details ---------------------- A customized directory structure is implemented as follows: - Default directories are configured in _defaults/src/build/settings/base.json - If the file `fbs_directories.json` in the root directory exists it will be read by fbs. Here, custom paths can be set, with the following options - python_dir : Directory of the python code - icons_dir : Directory of the icons - resources_dir: Directory of other resources.py - settings_dir: Directory of the fbs settings - freeze_config_dir: Configuration files for the freeze command - Eventually, the remaining fbs settings are read (either from the default or user configured path) I had to alter the check for the root directory, as it assumed a fixed directory structure. Now, it will check for the default settings path or the existence of the fbs_directories.json file. Testing ------- I tested the functionality on the fbs test app, that is created by the fbs tutorial. I tested the commands `run`, `freeze` and `installer` successfully. I tested only on windows though. Mac and Linux are yet untested. Closes mherrmann#268
Thank you for this PR. I'm a little weary of merging it because it complicates the code quite a bit. In the years since fbs has existed, this is also the first time that this requirement comes up. So I'm hesitant to make changes that (so far) one person has requested, but that will affect me and anybody else who touches fbs's code for the foreseeable future. I'm leaving this PR open for others, or in case the requirement comes up again. If you'd like to discuss another arrangement where your employer pays me to merge it into fbs, along with an fbs Pro license, let me know. I hope my stance on this is understandable. |
Thanks for looking into the PR. I can understand your concern about having changes which you are uncomfortable with. The only bigger changes is actually the function I am wondering though, that nobody asked for this feature yet as I think, that a packaging tool should have this flexibility and not dictate a certain file structure, which is also different from most of the python projects out there. I am okay though in having this PR open for future reference. This PR is solely for private reasons, so there's no company involved here ;) |
I find it odd that this requirement has only came up once. It was literally the first thing I thought of when looking into fbs! Would definitely add my +1 to this being merged. |
Also adding my +1 to this feature being added, I was equally surprised to find that nobody else has requested this feature. Thanks for your work on this PR Martin! |
This PR implements a custom directory structure for fbs.
Note: the previous directory structure is still used by default.
Implementation details
A customized directory structure is implemented as follows:
fbs_directories.json
in the root directory existsit will be read by fbs. Here, custom paths can be set, with the following options
(either from the default or user configured path)
I had to alter the check for the root directory, as it assumed a fixed
directory structure. Now, it will check for the default settings path
or the existence of the fbs_directories.json file.
Testing
I tested the functionality on the fbs test app, that is created by the
fbs tutorial. I tested the commands
run
,freeze
andinstaller
successfully. I tested only on windows though. Mac and Linux are
yet untested.
Closes #268