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

Value.Int should work with int32 #287

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Value.Int should work with int32 #287

wants to merge 5 commits into from

Conversation

pravic
Copy link
Member

@pravic pravic commented Feb 7, 2021

This applies to:

  • Value.Int() int32
  • Value.SetInt(int32)

fixes #286

cc @AshfordN

This applies to:

* `Value.Int() int32`
* `Value.SetInt(int32)`

refs #286
@pravic pravic self-assigned this Feb 7, 2021
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
@pravic
Copy link
Member Author

pravic commented Feb 7, 2021

That was intense.

@pravic
Copy link
Member Author

pravic commented Feb 8, 2021

@AshfordN Do you have any idea why it fails in AppVeyor?

sum := int32(0)
for i := 0; i < len(args); i++ {
sum += args[i].Int()
}

invalid operation: sum += args[i].Int() (mismatched types int32 and int)

@AshfordN
Copy link
Contributor

AshfordN commented Feb 8, 2021

Based on my tests, this is caused by trying to build the example(s) in this (value-int32) branch while linking against the current master branch. Now I'm not at all experienced with AppVeyor, but right around line 99 of the output, just before the examples are built, it seems as though the service is pulling in go-sciter from the master branch.
Keep in mind that this PR introduces breaking changes.

@pravic
Copy link
Member Author

pravic commented Feb 8, 2021

it seems as though the service is pulling in go-sciter from the master branch.

Apparently, so.

Keep in mind that this PR introduces breaking changes.

Yeah. I wonder whether we should introduce versioning finally.

@AshfordN
Copy link
Contributor

AshfordN commented Feb 8, 2021

Yeah. I wonder whether we should introduce versioning finally.

I think it makes sense, especially since this touches, what I consider to be, a core part of the package. This is likely to affect a lot of code bases.

@pravic pravic marked this pull request as draft February 8, 2021 14:46
@AshfordN
Copy link
Contributor

What is the status of the PR? As far as I see, this is a working branch. The only thing left to do, is to introduce versioning to avoid conflicts with existing code bases. Additionally, since versioning is now required starting with Go 1.16, I think now is a good time to implement it going forward.

@pravic
Copy link
Member Author

pravic commented Mar 29, 2021

The only thing left to do, is to introduce versioning to avoid conflicts with existing code bases.

Yep, this. Would you consider filing a PR for versioning?

@AshfordN
Copy link
Contributor

AshfordN commented Apr 2, 2021

No problem. I'll address it as soon as I can.

@c-smile c-smile marked this pull request as ready for review September 5, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

sciter.Value.Int() should return int32
2 participants