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

Feat: Angle parsing #6

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Feat: Angle parsing #6

wants to merge 7 commits into from

Conversation

RivMt
Copy link

@RivMt RivMt commented Oct 16, 2023

Thank you for your convenient package.

I added feature about parse strings to Angle instance.
It parses degrees, radians, and gradians using RegExp. For example, Angle.parse("360°") would be parsed to Angle that _storage is 0. You can find more examples in README.md on my PR.
And I upgraded min dart sdk version to 2.17 for conveninent use of enums. (previously 2.12) It's because these features uses enum to parse strings easilty.
Also, I created unit test about parsing strings.

Could you merge this PR and release new version of package?

Thanks,

Upgrade min dart sdk version to `2.17.0` for convenient use of enum
Add `AngleType` enum to handle angle properly
Update `README.md` with usage of parse angles
Add unit test for angle parsing
Copy link
Owner

@jim-ec jim-ec 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 contribution! I added a few comments, but all in all this looks good.

lib/angle_type.dart Outdated Show resolved Hide resolved
lib/angle_format.dart Outdated Show resolved Hide resolved
lib/angles.dart Show resolved Hide resolved
RivMt and others added 3 commits October 18, 2023 23:36
Remove unnecessary file `angle_format.dart`
Reformat `Angle.parse` constructor
@jim-ec
Copy link
Owner

jim-ec commented Oct 20, 2023

After locally checking out the code, I have some more issues with your code.

  • Please move the AngleUnit to the bottom of the lib/angles.dart file and check its documentation, there are some spelling errors.
  • Honestly, I am confused why the AngleUnit type is required at all. The API should simply expose a parsing function (which is a good addition by this PR), but as I see this, the AngleUnit enum is an intermediate type and should be replaced by e.g. a switch block in the parsing function.
  • What is the purpose of the radix?
  • The unit to parse should be unique. Strings like "360pi degrees" should fail to parse.
  • The second parameter to Angle.parse is somewhat nonsensical to me, after all the parse function's job is to parse the unit on its own.
  • Please add more unit tests which use different notations for floating points, e.g.: -4, 4., 4.3, +1e-2, -.12e-9, NaN, Infinity (all accepted by double.parse)

@RivMt
Copy link
Author

RivMt commented Oct 21, 2023

Thanks to your comment.

To be honest, I agree with you. But I couldn't think of a better way than the current implementation.

It's certainly possible to implement it using a switch statement, but I felt like Angle.parse would be excessively long. Granted, I don't think the way that doing it through AngleUnit is quite right, but I think it allows for a concise implementation.

The radix was introduced to convert the units of the sextant. It stands for 60, which is the number you multiply by when calculating degrees to minutes and minutes to seconds. Also, I don't think radix is actually a good name, but I couldn't think of anything better. In fact, radix is not necessary to radians and gradians. But like I said, I added radix as a member of AngleUnit to make the code as concise as possible in place of conditional statements.

Also, at the time I wrote it, I thought it would be nice to specify the units via the second argument to Angle.parse, but now that you mention it, I realize that most parse constructors (like double.parse) don't have such an argument, so I don't think the second argument is necessary.

In conclusion, I think it would be better to do it as a conditional statement as you suggest. If I were to write it, I think it would look something like below. But I am not certain that below code is concise and clean. Could you tell me if you have a better idea?

factory Angle.parse(String string) {
  int type = 0; // 0=degrees, 1=radians, 2=gradians
  if (string.contains(r"RegExp for degrees")) {
    type = 0;
  } else if (string.contains(r"RegExp for radians")) {
    type = 1;
  } // ...
  
  switch(type) {
    case 0:
      // Parse string as degrees
      return Angle.degrees(value);
    // ... other cases
  }
}

After I modifed the Angle.parse, I will fix bugs, update documentations and add new unit test.

Thank you for reading this.

@RivMt
Copy link
Author

RivMt commented Oct 24, 2023

The unit to parse should be unique. Strings like "360pi degrees" should fail to parse.

Could you explain why "360pi degrees" should be failed?
I think "360pi degrees" should be parsed as 1130.973...° (=360π°) because pi is not unit.

Or is there anything that I missed?

@jim-ec
Copy link
Owner

jim-ec commented Oct 25, 2023

  1. I not sure why u defer the switch block at all. Why not sth like this:
if ( /* Try to match regex for degrees */ ) {
    // Parse and return degrees
}
else if ( /* Try to match regex for multiples of PI */ ) {
    // Parse and return
}
// ...
  1. If an expression like 3pi deg parses, why not 3deg pi? In a sense, the deg is a factor of $\frac{\pi}{180}$. On the other hand, when I say that pi is a constant, than pi itself should also parse. In the end, I want simple angle parsing, not a parser for general product terms.

@RivMt
Copy link
Author

RivMt commented Nov 4, 2023

I understand.

I changed Angle.parse to use if to parse, just like you said. And AngleUnit is deleted.

I also modified it to not parse cases like 3pi deg.
So only cases like 3pi or 3pi rad will be parsed as .

In the unit tests, I added cases like NaN and infinity as mentioned earlier.

Is there anything else that I need to work on or that I still missed?

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.

2 participants