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

CreateKey and SetValue functions #211

Merged
merged 17 commits into from Mar 30, 2021
Merged

CreateKey and SetValue functions #211

merged 17 commits into from Mar 30, 2021

Conversation

rcorredera
Copy link
Contributor

No description provided.

@rcorredera
Copy link
Contributor Author

Hello, I'm not a C++ developer, please don't be hard on me :) I want to implement "setvalue" and "createKey" functions. If you have remarks or if you want to provide some help don't hesitate !

@rcorredera rcorredera marked this pull request as draft March 12, 2021 17:12
@sergiou87
Copy link
Member

No worries, C++ is already hard enough on all of us 🤣

Would you mind providing some context in the PR description? What it adds, why it's needed, any relevant implementation details…

Thank you!

@rcorredera
Copy link
Contributor Author

Hello,
I want to implement the possibility the create a registry key and to set a value. (RegCreateKeyEx and RegSetValueEx)
For the SetValue I only tried to implement the type REG_SZ & REG_DWORD.
By doing this I can use fully registry-js instead of winreg :)

@rcorrederalsi
fix ts lint
2518562
@rcorrederalsi
fix lint ts file.
538a990 
add unit tests.
fix SetValue function
manage only reg_sz & reg_dword
@rcorrederalsi
add unit tests
3867978
@rcorrederalsi
Copy link
Contributor

Hi @sergiou87, I've done the best I can. If you can take a look and tell me if it's okay for you. I also made the unit tests and it seems working.

@rcorredera rcorredera marked this pull request as ready for review March 17, 2021 16:15
sergiou87
sergiou87 requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I spotted a few changes you could make. I'll give it another review after you make the changes, but overall looks good! 😄

lib/registry.ts Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
@rcorrederalsi
Fix wording
fc18e92 
Remove null support on parameters (ts files)
@rcorrederalsi
fix invalid condition on createkey function
4f93169 
reorganize parameters check
@rcorrederalsi
change check on arg counts
7961535 
reorganize null check on args
@rcorrederalsi
attempt to fix indentation :crossfinger
54893cd
@rcorredera
Copy link
Contributor Author

Hi @sergiou87, I've pushed some changes, you can check again :) Thank for your time

rcorrederalsi
rcorrederalsi reviewed Mar 22, 2021
View reviewed changes
Copy link
Contributor

@rcorrederalsi rcorrederalsi left a comment

Choose a reason for hiding this comment

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

I replied in each section when details needed. I made the changes you asked for.

sergiou87
sergiou87 requested changes Mar 29, 2021
View reviewed changes
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Thanks for those changes @rcorredera!! And I'm sorry it took me so long to review it 😓

I found a few more issues around parameter checks.

src/main.cc Show resolved Hide resolved
src/main.cc Show resolved Hide resolved
src/main.cc Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
@rcorrederalsi
remove useless null check.
c3f8fff 
force string for value on setValue function , casting in c++ for dword still works.
fix wording
@rcorredera
Copy link
Contributor Author

Hi @sergiou87 ! Thank your for your time.
You were right about all your points. The complicated part to me was to determine if using only the string format for the value preoperty of the setValue function was a good strategie.
But as I only handled dword & reg_sz it seems to be okay.
Hope it will be good this time ! 🤞

sergiou87
sergiou87 requested changes Mar 29, 2021
View reviewed changes
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

On a quick review of your latest changes, I think everything is good now! Only found a couple of typos, but nothing else.

I will review it again tomorrow with fresh eyes and give it the final ✅  if I can't find anything else 😄

Awesome work!!

src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
rcorredera and others added 2 commits March 30, 2021 11:09
@rcorredera @sergiou87
fix wording
b720a5a 
Co-authored-by: Sergio Padrino <sergiou87@github.com>
@rcorredera @sergiou87
fix wording
5d249ff 
Co-authored-by: Sergio Padrino <sergiou87@github.com>
sergiou87
sergiou87 approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Everything is looking good now, thank you so much for your contribution! ❤️

@sergiou87 sergiou87 merged commit 7e63e09 into desktop:master Mar 30, 2021
@rcorredera
Copy link
Contributor Author

Yeah ! :) Thank you for your time ! Do you know when you will release a version including these changes ?

@sergiou87
Copy link
Member

I will talk to the team about it and let you know 😄

@sergiou87 sergiou87 mentioned this pull request Apr 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcorrederalsi rcorrederalsi rcorrederalsi left review comments

@sergiou87 sergiou87 sergiou87 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
@rcorredera @sergiou87 @rcorrederalsi