Refactor Code Generator to New Module and add CLI #297
Conversation
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
|
@amolenaar I cleaned up a handful of mypy errors that I knew how to fix. I am not sure what to do with the remaining ones, should I put ignore comments on these lines? |
|
I think that’s fine for now. There was a reason I did not run Mypy on the utils folder 🙄. |
Makefile
Outdated
| gaphor/core/modeling/coremodel.py: models/Core.gaphor models/Core.override utils/model/gen_uml.py utils/model/override.py utils/model/writer.py | ||
| utils/model/build_core.py && black $@ && mypy gaphor/core/modeling | ||
| gaphor/core/modeling/coremodel.py: models/Core.gaphor models/Core.override gaphor/codegen/autocoder.py gaphor/codegen/override.py gaphor/codegen/writer.py | ||
| gaphor/codegen/codegen.py Core && black $@ && mypy gaphor/core/modeling |
There was a problem hiding this comment.
I would use specific arguments for the model, overrides and output files.
There was a problem hiding this comment.
@amolenaar Thanks so much for the feedback, I guess I was trying to make things a little too tricky! 💯 Please let me know if you like the updated version better.
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
gaphor/codegen/codegen.py
Outdated
| args = parser.parse_args() | ||
| modelfile: Path = args.modelfile | ||
| outfile: Path = args.outfile | ||
| if str(modelfile) == modelfile.name: |
There was a problem hiding this comment.
I think we do not need the if-blocks. Let's just be specific in the arguments instead. If we can not resolve a file, just exit.
There was a problem hiding this comment.
Ok, I'm good with that! 😸
| @@ -17,32 +17,33 @@ def close(self): | |||
|
|
|||
|
|
|||
| def test_loading(): | |||
| model_file = distribution().locate_file("tests/test-model.gaphor") | |||
| model_file = distribution().locate_file("gaphor/codegen/tests/test-model.gaphor") | |||
There was a problem hiding this comment.
Should we move the test model to test-diagrams?
I know it's not ideal, since unit tests are packaged with the production code. Now there are several tests already that break because they depend on diagrams outside the source (code) tree.
Either we should move all these tests out of the source tree into /tests/ or include all test diagrams in the source tree. I'd opt for the first.
There was a problem hiding this comment.
Thanks, I agree that we should keep the models out of the source packages. Thanks for catching that! 👍 Since you created a new models package, I thought it made sense to move test-diagrams to test-models and nest it under where the rest of the models are located. What do you think about that?
|
Despite my comments, I think the PR is good to go. It's a major improvement, so it belongs on master. 👍 |
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
|
I’m not sure if test-models should belong under models or if it’s more like a sibling. |
|
Oh well... |
|
Parent: #138 |
This is a refactoring as part of in the future supporting SysML. It takes the util modules for parsing the gaphor models and generating the data model, and moves them to a separate codegen module. I also used the builtin argparse to add a CLI.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information