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.

Sign up for GitHub

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

Jump to bottom

Issue 713: Support variable buffer sizes in allocator #841

Merged
merged 2 commits into from
Dec 6, 2021
Merged

Issue 713: Support variable buffer sizes in allocator #841

merged 2 commits into from
Dec 6, 2021

Conversation

kanigsson
Copy link
Collaborator

@kanigsson kanigsson commented Nov 8, 2021 •
edited
Loading

For #713

treiher
treiher requested changes Nov 9, 2021
View reviewed changes
Copy link
Collaborator

@treiher treiher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design should be changed. The integration data should not be part of the model, but a separate data structure. The integration file is parsed in the specification parser and the integration data is passed to the code generator as a separate parameter.

rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/session.py Outdated Show resolved Hide resolved
rflx/model/session.py Outdated Show resolved Hide resolved
rflx/specification/parser.py Outdated Show resolved Hide resolved
rflx/specification/parser.py Outdated Show resolved Hide resolved
rflx/specification/parser.py Outdated Show resolved Hide resolved
@kanigsson kanigsson force-pushed the issue_713 branch 3 times, most recently from 82b7134 to 343e7ea Compare November 17, 2021 03:00
treiher
treiher requested changes Nov 24, 2021
View reviewed changes
rflx/specification/integration.py Outdated Show resolved Hide resolved
rflx/specification/integration.py Outdated Show resolved Hide resolved
rflx/specification/integration.py Outdated Show resolved Hide resolved
rflx/specification/integration.py Outdated Show resolved Hide resolved
rflx/specification/integration.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tests/integration/session_integration/test.rfi Outdated Show resolved Hide resolved
tests/integration/session_integration/test.rflx Outdated Show resolved Hide resolved
rflx/specification/integration.py Outdated Show resolved Hide resolved
tests/unit/specification/parser_test.py Outdated Show resolved Hide resolved
@kanigsson kanigsson force-pushed the issue_713 branch 2 times, most recently from 24b1b2b to 28ac121 Compare November 26, 2021 08:14
treiher
treiher requested changes Nov 26, 2021
View reviewed changes
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
tests/unit/generator_test.py Outdated Show resolved Hide resolved
tests/integration/session_integration_multiple/msg.rflx Outdated Show resolved Hide resolved
tests/integration/session_integration_multiple/config.yml Outdated Show resolved Hide resolved
tests/integration/session_integration/config.yml Outdated Show resolved Hide resolved
tools/generate_spark_test_code.py Outdated Show resolved Hide resolved
treiher
treiher reviewed Nov 29, 2021
View reviewed changes
rflx/integration.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
treiher
treiher requested changes Nov 29, 2021
View reviewed changes
rflx/integration.py Outdated Show resolved Hide resolved
rflx/integration.py Outdated Show resolved Hide resolved
rflx/integration.py Outdated Show resolved Hide resolved
tests/integration/session_integration_multiple/test.rflx Outdated Show resolved Hide resolved
tests/integration/session_integration_multiple/b.rflx Outdated Show resolved Hide resolved
tests/unit/integration_test.py Outdated Show resolved Hide resolved
tests/integration/specification_model_test.py Outdated Show resolved Hide resolved
@treiher treiher changed the title support variable buffer sizes in allocator Issue 713: Support variable buffer sizes in allocator Nov 29, 2021
@treiher treiher mentioned this pull request Nov 30, 2021
Open
treiher
treiher requested changes Nov 30, 2021
View reviewed changes
tests/unit/integration_test.py Show resolved Hide resolved
rflx/integration.py Outdated Show resolved Hide resolved
treiher
treiher requested changes Dec 2, 2021
View reviewed changes
doc/User-Guide.adoc Show resolved Hide resolved
doc/User-Guide.adoc Outdated Show resolved Hide resolved
rflx/specification/parser.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
rflx/generator/allocator.py Outdated Show resolved Hide resolved
treiher
treiher requested changes Dec 3, 2021
View reviewed changes
rflx/specification/parser.py Outdated Show resolved Hide resolved
treiher
treiher reviewed Dec 3, 2021
View reviewed changes
Copy link
Collaborator

@treiher treiher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

Before merging this PR, the tasks in #713 should be updated. As discussed in our last meeting, #713 should be kept open as a meta ticket (so the linking to this PR must be removed) and new tickets should be opened for the open tasks.

@kanigsson
Copy link
Collaborator Author

Is it possible to remove a link to an issue from a PR? I couldn't find any info on that ...

@kanigsson
Copy link
Collaborator Author

Also, from your other change on the Process documentation, I think you want me to squash the commits into a single one, right?

@treiher
Copy link
Collaborator

treiher commented Dec 3, 2021

Is it possible to remove a link to an issue from a PR? I couldn't find any info on that ...

It is possible to remove linked issues in the Linked issues section in the right pane (below Reviewers). If you don't have the permissions for that, please let me know.

Also, from your other change on the Process documentation, I think you want me to squash the commits into a single one, right?

Squashing is not mandatory, but as described in the documentation, we prefer that it is done.

@kanigsson
Copy link
Collaborator Author

kanigsson commented Dec 6, 2021 •
edited
Loading

I have squashed everything. I don't seem to have the rights to remove the link, so if you could do it for me (or give me the rights) that would be great. Alternatively, we could just reopen the issue afterwards?

Also, I had to add a new commit to fix a testcase (after rebase on main), so I am requesting your review again.

@treiher
Copy link
Collaborator

treiher commented Dec 6, 2021

I have squashed everything. I don't seem to have the rights to remove the link, so if you could do it for me (or give me the rights) that would be great. Alternatively, we could just reopen the issue afterwards?

I double-checked, you should already have the necessary rights. If the problem is that the linked issue is greyed out in the drop-down list, the reason is the Closes #713 in the PR description. So if you edit the description, the linking should be automatically removed. (GitHub seems to have changed the behavior recently. In the past, removing the Closes #... was not sufficient and not necessary for removing the linking manually.)

@kanigsson kanigsson merged commit 3d5bee1 into main Dec 6, 2021
@kanigsson kanigsson deleted the issue_713 branch December 6, 2021 08:30
@kanigsson
Copy link
Collaborator Author

That worked, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@senier senier senier left review comments

@treiher treiher treiher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kanigsson @treiher @senier