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

Feature: + supported in ORIGIN #70

Merged
merged 11 commits into from
Apr 4, 2022
Merged

Feature: + supported in ORIGIN #70

merged 11 commits into from
Apr 4, 2022

Conversation

Dajamante
Copy link
Contributor

@Dajamante Dajamante commented Apr 2, 2022 •
edited
Loading

flip-link does now accept additions for Origin as in this issue.
fixes #65.

This was referenced Apr 2, 2022
Closed
Merged
Urhengulas
Urhengulas requested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

Thank you for your work @Dajamante! It looks very good overall, there are just a few nitpicks.

Regarding the println statements you added: I understand that you added them for debugging purposes, which is a good thing to have. But I don't think they should appear in the normal output, since, as far as I understand, they are not meant for the user of flip-link, but rather a developer working on flip-link.
Therefore please change them to log::debug! (or another log-level if you think it is better suited), since then they won't appear by default, but can be enabled via the RUST_LOG environment variable.

Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@Dajamante
Copy link
Contributor Author

Thank you for your work @Dajamante! It looks very good overall, there are just a few nitpicks.

Regarding the println statements you added: I understand that you added them for debugging purposes, which is a good thing to have. But I don't think they should appear in the normal output, since, as far as I understand, they are not meant for the user of flip-link, but rather a developer working on flip-link. Therefore please change them to log::debug! (or another log-level if you think it is better suited), since then they won't appear by default, but can be enabled via the RUST_LOG environment variable.

I am really sorry about the println!() statements, those should have been removed as you pointed out... I will address the rest of your comments shortly!

Urhengulas
Urhengulas requested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my review! I added two more, small, questions regarding the doc-comments.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@Dajamante
Copy link
Contributor Author

Dajamante commented Apr 4, 2022 •
edited
Loading

Thank you for addressing my review! I added two more, small, questions regarding the doc-comments.

No really, kudos to you 😄 , that was an excellent review! I addressed your two comments.

Urhengulas
Urhengulas requested changes Apr 4, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Urhengulas
Urhengulas requested changes Apr 4, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
Urhengulas
Urhengulas reviewed Apr 4, 2022
View reviewed changes
src/main.rs Show resolved Hide resolved
Urhengulas
Urhengulas approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

bors d+

@bors
Copy link
Contributor

bors bot commented Apr 4, 2022

✌️ Dajamante can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@Dajamante
Copy link
Contributor Author

v Dajamante can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 4, 2022

Build succeeded:

@bors bors bot merged commit 4937225 into knurling-rs:main Apr 4, 2022
@Urhengulas Urhengulas changed the title - feature: + supported in ORIGIN Feature: + supported in ORIGIN Apr 4, 2022
@Urhengulas Urhengulas changed the title Feature: + supported in ORIGIN Feature: + supported in ORIGIN Apr 4, 2022
@Dajamante Dajamante deleted the plus_sign_in_origin branch April 4, 2022 16:18
@joloev joloev mentioned this pull request May 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Urhengulas Urhengulas Urhengulas 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.

Does not support math in RAM origin
2 participants
@Dajamante @Urhengulas