Skip to content

Comments

class field can be replaced by local variable#8809

Merged
pdurbin merged 1 commit intoIQSS:masterfrom
noobyu6:master
Sep 8, 2022
Merged

class field can be replaced by local variable#8809
pdurbin merged 1 commit intoIQSS:masterfrom
noobyu6:master

Conversation

@noobyu6
Copy link
Contributor

@noobyu6 noobyu6 commented Jun 20, 2022

What this PR does / why we need it:
class fields can be replaced by local variables which can save memory
Which issue(s) this PR closes:

Special notes for your reviewer:
no
Suggestions on how to test this:
code review
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no
Is there a release notes update needed for this change?:
reduce memory usage
Additional documentation:

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 19.718% when pulling 68be767 on noobyu6:master into 21ac7e1 on IQSS:master.

@scolapasta
Copy link
Contributor

Hi @noobyu6 , for this next sprint we are catching up on Community PRs. Would you mind updating/ refreshing thiis PR from develop? Thanks!

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@pdurbin
Copy link
Member

pdurbin commented Sep 8, 2022

As discussed at standup, we're comfortable with the same person doing review and QA. I'm merging this. Again, it's a trivial change and the SWORD API tests pass.

@pdurbin
Copy link
Member

pdurbin commented Sep 8, 2022

@noobyu6 I'm very sorry but I merged this pull request without noticing that it was targeting the "master" branch rather than the "develop" branch. Can you please create a new pull request that targets the "develop" branch? Thanks.

@noobyu6
Copy link
Contributor Author

noobyu6 commented Sep 9, 2022

I see the same situation exists in the master branch, so merging into the master branch is also ok

@pdurbin
Copy link
Member

pdurbin commented Sep 9, 2022

@noobyu6 no. I reverted the master branch (see #8967). It comes down to our process. All PRs are merged into develop. Then at release time we merge develop into master. So please make a new PR against develop if you want this change. Sorry for the inconvenience.

@noobyu6
Copy link
Contributor Author

noobyu6 commented Sep 9, 2022

@pdurbin I have made a new PR (see #8970).

@pdurbin pdurbin added this to the 5.12 milestone Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants