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

Add/rename electric quantities #1444

Merged
merged 11 commits into from
Dec 28, 2024
Merged

Add/rename electric quantities #1444

merged 11 commits into from
Dec 28, 2024

Conversation

McNeight
Copy link
Contributor

@McNeight McNeight commented Dec 9, 2024 •
edited by angularsen
Loading

Renamed quantities to have prefix "Electric", marking old name as obsolete and adding new duplicate quantities:

  • ApparentEnergy -> ElectricApparentEnergy
  • ApparentPower -> ElectricApparentPower
  • Capacitance -> ElectricCapacitance
  • ReactiveEnergy -> ElectricReactiveEnergy
  • ReactivePower -> ElectricReactivePower
  • ElectricPotentialAc
  • ElectricPotentialDc
  • ElectricAdmittance (obsolete due to being a complex number, not supported by UnitsNet, recommending ElectricReactance and ElectricSusceptance instead)

Decided against adding ElectricImpedance, also being a complex number.

New:

  • Add ElectricReactance
  • Add ElectricSusceptance
  • Add EletricCurrent = ElectricApparentPower / ElectricPotential
  • Add ElectricPotential = ElectricApparentPower / ElectricCurrent
  • ElectricAdmittance:
    • Add prefixes: Kilo, Mega, Giga, Tera
    • Add Mho unit, equivalent to Siemen
  • ElectricConductance:
    • Add prefixes: Mega, Giga, Tera
    • Add Mho unit, equivalent to Siemen
  • EletricResistance:
    • Add prefixes: Nano

angularsen
angularsen requested changes Dec 9, 2024
View reviewed changes
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Not sure if this was ready to review yet, but a few minor comments about the texts.

Common/UnitDefinitions/Capacitance.json Outdated Show resolved Hide resolved
Common/UnitDefinitions/ElectricPotentialAc.json Outdated Show resolved Hide resolved
@McNeight
Electric
c13f66e 
Rearranged some of the electrical units, all with the prefix "Electric", and labeled the following classes as obsolete:
* ApparentEnergy -> ElectricApparentEnergy
* ApparentPower -> ElectricApparentPower
* Capacitance -> ElectricCapacitance
* ReactiveEnergy -> ElectricReactiveEnergy
* ReactivePower -> ElectricReactivePower
Added classes for ElectricReactance and ElectricSusceptance.
Labeled ElectricPotentialAc and ElectricPotentialDc as obsolete per discussion.
Labeled ElectricAdmittance as obsolete due to it being a complex number.
Decided against adding ElectricImpedance for the same reason.
Added math to ElectricCurrent.extra.cs and ElectricPotential.extra.cs to support ElectricApparentPower
Added math for ElectricApparentPower.extra.cs
Minor correction to formula in comment of Power.extra.cs
@McNeight
Electric
e4be1e9 
Neglected to generate code and build.
Error with trying to sign the assembly, but otherwise UnitsNet appears to build.
UnitsNet.Tests, however, seems to have problems.
@McNeight
Electric
f63fec8 
Revised ObsoleteText messages to be more useful.
Added correct testing code.
Added ElectricImpedance class; even though it's a complex number, it "rounds out" the other five units of:
* Admittance
* Conductance
* Reactance
* Resistance
* Susceptance
Removed excess multiplication operators for ElectricApparentPower.
@McNeight
Electric
96a8ebf 
Force the System.Formats.Asn1 package to the latest version to see if that clears the Azure DevOps error with CodeGen.
@McNeight
Copy link
Contributor Author

Should I make a second PR against default/v6, or would the code change here be merged into the next version as well?

@angularsen
Copy link
Owner

@McNeight It will eventually be merged into v6 when syncing master into v6, but it is very helpful if you coulg do a separate PR for v6 simply to reduce the amount of merge conflicts 🙏

Let's hold until this is merged though, is it ready to review now?

@McNeight
Copy link
Contributor Author

Not a problem!

I believe it's ready to review for v5, if you want to mark ElectricPotentialAc and Dc as obsolete in v5.

If you wanted to save the breaking change for v6, then I'll need to rework the commits a pull requests, but the content should be ready for review.

angularsen
angularsen requested changes Dec 25, 2024
View reviewed changes
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

This is looking pretty good now, my only remaining comments are regarding ElectricAdmittance and ElectricImpedance.

CodeGen/CodeGen.csproj Outdated Show resolved Hide resolved
Common/UnitDefinitions/ElectricAdmittance.json Show resolved Hide resolved
Common/UnitDefinitions/ElectricImpedance.json Show resolved Hide resolved
CodeGen/Generators/UnitsNetGen/NumberExtensionsGenerator.cs Show resolved Hide resolved
lipchev
lipchev reviewed Dec 27, 2024
View reviewed changes
Common/UnitDefinitions/ElectricReactiveEnergy.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the Joule an applicable / useful unit for this quantity? If yes- then it would allow us to have an SI unit for #1463

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I actually meant to comment on #1467 but got my tabs confused..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively VoltampereReactiveSecond would also do the trick, if you think it's a better fit (note, we don't have any rules about it- if you think neither units make sense, we could just [Skip] the tests and deal with it later).

Copy link
Owner

Choose a reason for hiding this comment

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

I'll merge this now, we can still add this unit later if it makes sense.

McNeight and others added 3 commits December 27, 2024 17:14
@McNeight
Electric
0b20a6e 
Fixed base dimensions for ReactiveEnergy and ElectricReactiveEnergy.
Thanks @lipchev!
@angularsen
Merge remote-tracking branch 'origin/master' into Electric
39654da 
# Conflicts:
#	Common/UnitEnumValues.g.json
#	UnitsNet.Tests/GeneratedCode/TestsBase/ElectricAdmittanceTestsBase.g.cs
#	UnitsNet.Tests/GeneratedCode/TestsBase/ElectricConductanceTestsBase.g.cs
#	UnitsNet.Tests/GeneratedCode/TestsBase/ElectricResistanceTestsBase.g.cs
@angularsen
regen
7d393bb
@angularsen angularsen changed the title Electric Add/rename electric quantities Dec 28, 2024
@angularsen angularsen merged commit 6b05cc8 into angularsen:master Dec 28, 2024
1 check was pending
@angularsen
Copy link
Owner

Thanks @McNeight ! Nuget should be out shortly.

Release UnitsNet/5.65.0 · angularsen/UnitsNet

Please see the remaining question above regarding SI unit.

Other than that, we should be ready for the v6 PR next.

@angularsen angularsen mentioned this pull request Dec 28, 2024
Merged
@angularsen
Copy link
Owner

@McNeight Just a heads up regarding v6 PR:

angularsen added a commit that referenced this pull request Dec 28, 2024
@angularsen
💚Fix nanoFramework build, replace <see cref=""> with <c> in xmldoc (#…
b3f04ec 
…1478)

Regression in #1444 

nanoFramework is built differently and each quantity is its own
assembly, with no reference to other quantities. This gave a build
warning, that is treated as build error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lipchev lipchev lipchev left review comments

@angularsen angularsen angularsen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@McNeight @angularsen @lipchev