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

Refactor #62

Draft
wants to merge 3 commits into
base: v2
Choose a base branch
from
Draft

Refactor #62

wants to merge 3 commits into from

Conversation

ysmilda
Copy link
Contributor

@ysmilda ysmilda commented Dec 9, 2022

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

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible, they don't break existing deployments.
  • Testing: The changes are tested.
  • Documentation: Relevant documentation is added or updated.

@ysmilda ysmilda self-assigned this Dec 9, 2022
Copy link
Contributor

@adriansmares adriansmares left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
end-device/EU_863_870.yml Show resolved Hide resolved
Copy link
Contributor

@adriansmares adriansmares left a 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.

.vscode/settings.json Outdated Show resolved Hide resolved
@@ -0,0 +1,77 @@
package utils
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

internal/model/end-device-frequency-plan.go Outdated Show resolved Hide resolved
@ysmilda ysmilda changed the base branch from master to v2 January 31, 2023 09:31
@ysmilda ysmilda force-pushed the refactor branch 3 times, most recently from 07cc093 to 662831e Compare January 31, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants