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 String.Format #12

Merged
merged 4 commits into from
Jun 15, 2018
Merged

Add String.Format #12

merged 4 commits into from
Jun 15, 2018

Conversation

ishvedov
Copy link
Contributor

@ishvedov ishvedov commented Jun 1, 2018 •
edited
Loading

Description

Add String.Format function

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: ishvedov ivan.shvedov@gmail.com

@nfbot
Copy link
Member

nfbot commented Jun 1, 2018

Hi @ishvedov,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@josesimoes
Copy link
Member

Was testing this with the following code:

    public class Program
    {
        public static void Main()
        {
            try
            {
                DateTime date1 = new DateTime(2009, 7, 1);
                TimeSpan hiTime = new TimeSpan(14, 17, 32);
                double hiTemp = 62.1;
                TimeSpan loTime = new TimeSpan(3, 16, 10);
                double loTemp = 54.8;

                string result1 = String.Format("Temperature on {0:d}:\n{1,11}: {2} degrees (hi)\n{3,11}: {4} degrees (lo)",
                                               date1, hiTime, hiTemp, loTime, loTemp);
                Console.WriteLine(result1);
                Console.WriteLine("");

                string result2 = String.Format("Temperature on {0:d}:\n{1,11}: {2} degrees (hi)\n{3,11}: {4} degrees (lo)",
                                               new object[] { date1, hiTime, hiTemp, loTime, loTemp });
                Console.WriteLine(result2);


                Thread.Sleep(Timeout.InfiniteTimeSpan);
            }
            catch (Exception ex)
            {
                
            }
        }
    }

but it's throwing at me:
#### Exception System.ArgumentException - 0x00000000 (1) ####
#### Message: Format error: wrong simbol in {}
#### System.String::Format [IP: 0153] ####
#### NFApp327.Program::Main [IP: 006f] ####

@ishvedov
Copy link
Contributor Author

ishvedov commented Jun 4, 2018

Not implemented alignment. I'll make it today.

@josesimoes
Copy link
Member

Oh... I see... for sure we won't need the full blown Format() so it's OK to have a lightweight version.
Just need to make it clear what is supported and what isn't.

@josesimoes
Copy link
Member

After the latest commit I'm getting:

Temperature on 07/01/2009: 14:17:32: 62.100000000000001 degrees (hi) 03:16:10: 54.799999999999997 degrees (lo)⠴

Like I said above, because this is a constrained resource platform no one would be expected to see a fully implemented String.Format
I suggest we decide on what should be me available and concentrate on that. What do you think?

@ishvedov
Copy link
Contributor Author

Problem with broken string.this[] in new firmwares. I wrote it in Slack

@ishvedov
Copy link
Contributor Author

Wait for resolve nanoframework/Home#343

@josesimoes
Copy link
Member

Blocked by nanoframework/Home#343.

@josesimoes
Copy link
Member

The output is now the expected one:

hiTemp62.100000000000001
loTemp54.799999999999997
Temperature on 07/01/2009:
   14:17:32: 62.100000000000001 degrees (hi)
   03:16:10: 54.799999999999997 degrees (lo)

josesimoes added a commit to Eclo/nf-interpreter that referenced this pull request Jun 13, 2018
@josesimoes
Add support to String.Format
604c503 
- This is required to use nanoFramework.CoreLibrary after nanoframework/CoreLibrary#12 is merged

Signed-off-by: José Simões <jose.simoes@eclo.solutions>
@josesimoes josesimoes mentioned this pull request Jun 13, 2018
Merged
10 tasks
@josesimoes
Copy link
Member

After nanoframework/nf-interpreter#743 the output is now 100% correct when compared with the full .NET output:

hiTemp62.1
loTemp54.8
Temperature on 07/01/2009:
   14:17:32: 62.1 degrees (hi)
   03:16:10: 54.8 degrees (lo)

josesimoes
josesimoes approved these changes Jun 13, 2018
View reviewed changes
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM

@ishvedov
Improvments for PadLeft, PadRight
f8bfad4 
Speed up to 5 times, less memory usage.
@ishvedov
Copy link
Contributor Author

I did it.

josesimoes
josesimoes approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Nice improvement! Again: LGTM.

@josesimoes josesimoes merged commit 85ad030 into nanoframework:develop Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josesimoes josesimoes josesimoes approved these changes

Assignees
No one assigned
Labels
Type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ishvedov @nfbot @josesimoes