-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactor #62
base: v2
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably shouldn't target master
, given that older TTS versions point there, and we keep the old plans under the legacy
path now.
c8a74af
to
158d637
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, very nice job.
I think we should target a new v2
branch (that forks off master
) in order to avoid existing deployments.
@@ -0,0 +1,77 @@ | |||
package utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a local go.mod
/ go.sum
for this particular tool, like we have for tools
/test
in lorawan-stack
. Otherwise we will have import cycles when we try to use things like the schema and models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I countered the import cycles by using unnamed copy of the structure in the utils. Not quite sure how to set it up with a go.mod
at that level. Couldn't get imports to work into the main module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed adding some context here. My proposal was to actually make the models part of a public (not internal
) package that we can import into lorawan-stack
, such that we avoid having the models in two places. If we want to do that, the package exposed by lorawan-frequency-plans
should not import lorawan-stack
, which why I proposed that we decouple the model from the docs and schema generation.
How this works in lorawan-stack
is that the root contains the public go.mod
/go.sum
and the subfolder contains its own set of go.mod
/ go.sum
. Since the tools would import the top level model definitions, imports should work. It is indeed the case that the reverse is not really possible (you cannot import the tool into the root project, but that's fine).
07cc093
to
662831e
Compare
Summary
This PR reworks the Frequency plans repo to support and fix the following issues:
Closes #51
Closes #41
Closes #4
Closes #38
Changes
Notes for Reviewers
...
Checklist