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

qt: Prefix makefile variables with QT_ #23242

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

promag
Copy link
Member

@promag promag commented Oct 9, 2021

Improves consistency and readability if future QML variables are added.

@hebasto
Copy link
Member

hebasto commented Oct 9, 2021

Concept ACK. Use scripted-diff?

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

This PR focuses on improving the consistency and readability of Makefile.qt.include by renaming the variable per their proper function and usage. Since there is a high probability of introducing new QML variables in the future, this is a small but necessary change.

As we are on the matter of variable naming consistency, I want to add something, though. In the Makefile.qt.include on lines 292 and 324. Shouldn’t the variable BITCOIN_RS also be renamed to BITCOIN_QT_RS?

@promag
Copy link
Member Author

promag commented Oct 9, 2021

I’ll take a look @shaavan, thanks.

@laanwj
Copy link
Member

laanwj commented Oct 13, 2021

No objections, this probably improves the variable naming. Checked that none of the old names remain.

Code review ACK e8b6e7c

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e8b6e7c

Improves consistency and readability if future QML variables are added.

-BEGIN VERIFY SCRIPT-
sed -i \
    -e 's/RES_ANIMATION/QT_RES_ANIMATION/g' \
    -e 's/RES_FONTS/QT_RES_FONTS/g' \
    -e 's/RES_ICONS/QT_RES_ICONS/g' \
    -e 's/BITCOIN_RC/BITCOIN_QT_RC/g' \
    src/Makefile.qt.include
-END VERIFY SCRIPT-
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK eb04bad, only suggested changes, and script-diff used.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Code Review ACK eb04bad

Changes since my last review:

  • Renamed BITCOIN_RC -> BITCOIN_QT_RC
  • Used scripted-diff in commit message

@maflcko maflcko merged commit 5a044da into bitcoin:master Oct 14, 2021
@promag promag deleted the 2021-10-qt-makefile branch October 14, 2021 12:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 14, 2021
hebasto added a commit to bitcoin-core/gui-qml that referenced this pull request Oct 19, 2021
Pull request description:

  Sync with the main repo up to bitcoin-core/gui@ef59692.

  This sync includes:
  - bitcoin/bitcoin#23242 ported from #49
  - bitcoin/bitcoin#23286 ported from #58

  Parent commits:
  ```
  $ git rev-parse 8967f19^@
  8cfb3cb
  ef59692
  ```

ACKs for top commit:
  jarolrod:
    ACK 8967f19

Tree-SHA512: c387e638862afe5fd4711d9c1aad36165d04f328612c9b74b35a50b477933e6c6b8cf0674caf3b02e24a22d8598aee7e880baf4c3879c629f71a92af31714548
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants