Closed nadchif closed 4 years ago
Hi @nadchif i would like to contribute.
@Mayank2901 Sure, please go ahead ☺
HI Nadchif, I would like to contribute. Your contributing guide is really detailed - great job on it!
uh-oh, I forgot to @nadchif in my original comment. I would like to contribute, please
@nourmab13 thank you. Please go ahead ☺
This project is gathering so much contribution! Awesome to all. and @nadchif
FYI - I'm working on Roman <--> Decimal
Thats a nice one! looking forward to your contribution 😄
Hey @nadchif, I'm completed and tested Decimal-> Roman. Two questions for you: (1) Does it make sense to go ahead and submit pull request for this conversion alone, or shall I wait until I complete the Roman->Decimal and submit both together?
@nadchif here is the second item: May I please get your input on setting limits on the size of the decimal being converted? By definition, Roman numerals are not meant for massive numbers; thus M (1000) as the largest. So if you want to see 50K, it will print to 50 "M's"; my sense is to set a limit of perhaps 4, possibly 5 digits. Browserling doesn't appear to set a practical limit (I didn't try a million yet). I dislike the idea of allowing someone to print out a thousand "M's" just because they can. Also, someone may make an error and type in a huge number without meaning to, etc. Thoughts??
Hey @nadchif, I'm completed and tested Decimal-> Roman. Two questions for you: (1) Does it make sense to go ahead and submit pull request for this conversion alone, or shall I wait until I complete the Roman->Decimal and submit both together?
Hey @nourmab13 I'm glad to hear you are done with the Decimal -> Roman conversion. 👏 You may go ahead and work on the Roman -> Decimal. No pressure, no rush 😄
@nadchif here is the second item: May I please get your input on setting limits on the size of the decimal being converted? By definition, Roman numerals are not meant for massive numbers; thus M (1000) as the largest. So if you want to see 50K, it will print to 50 "M's"; my sense is to set a limit of perhaps 4, possibly 5 digits. Browserling doesn't appear to set a practical limit (I didn't try a million yet). I dislike the idea of allowing someone to print out a thousand "M's" just because they can. Also, someone may make an error and type in a huge number without meaning to, etc. Thoughts??
😆 This is a valid concern. I think it would be a great idea to implement a limit. If you plan on communicating back to the user, here is a snippet that might help:
const Dialogs = brackets.getModule('widgets/Dialogs');
Dialogs.showModalDialog('',
'{Add your Dialog Title Here}',
'{Add your dialog message here}');
}
Thanks, @nadchif ; I'll go ahead and start working on the Roman --> Decimal conversion. Thanks for the snippet - I leveraged same from some of your code when I checked that the input is all digits for the decimal conversion. I appreciate that you are so helpful!
It's a pleasure 🙏
Happy Coding! 💻
Hi @nadchif - I created a pull request, and I see it on my fork, not on the main repository (is that correct?). I just wanted to confirm that I submitted it correctly. Thanks!
@nourmab13 yooohoooo! 👏 😁
The pull request you opened, will update your fork's master branch by merging the latest commits from this master.
Please go ahead and merge that PR into your master. Once you are done, you can create pull request using this link https://github.com/nadchif/adobe-brackets-encode-decode/compare/master...nourmab13:master
Hi @nadhif - thanks for the help; I think something is not right. The pull request shows zero changed files - and I don't see where my changes are in the source in my repository in git. This suggests my changes are still local (I do see them). I'll review the documentation again tomorrow; I thought I followed it, but clearly not 😊
On Sun, Jun 14, 2020 at 4:36 PM Dan Chif notifications@github.com wrote:
@nourmab13 https://github.com/nourmab13 yooohoooo! 👏 😁 The pull request you opened, will update your fork's master branch https://github.com/nourmab13/adobe-brackets-encode-decode by merging the latest commits from this master https://github.com/nadchif/adobe-brackets-encode-decode.
Please go ahead and merge that PR into your master. Once you are done, you can create pull request using this link master...nourmab13:master https://github.com/nadchif/adobe-brackets-encode-decode/compare/master...nourmab13:master
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nadchif/adobe-brackets-encode-decode/issues/26#issuecomment-643818927, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKOU7BFPS4Q64BGALLR2KH3RWUYFNANCNFSM4M3U2MEQ .
-- Nourma Bumgarner CSM, PMP
Hi again. Please make sure you
git commit -m "description message"
git push
I think that would resolve this problem
Hi Dan, I tried to resubmit and I see that the build failed (and had also noted this when I ran npm test that it failed) because it's looking for roman.spc.js in the /spec folder. Do I need to write a test myself (following the instructions in the "Write more tests for the encoders and decoders #28" issue) before trying to submit my code?
Thanks!
On Mon, Jun 15, 2020 at 4:06 AM Dan Chif notifications@github.com wrote:
Hi again. Please make sure you
- commit your local changes
git commit -m "description message"
- Then push the changes to your fork's master branch
git push
I think that would resolve this problem
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nadchif/adobe-brackets-encode-decode/issues/26#issuecomment-643972074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKOU7BDPNL22ELWLUCQK4XLRWXJAPANCNFSM4M3U2MEQ .
-- Nourma Bumgarner CSM, PMP
We need more encoders/decoders, ie String to Hex, String to MD5 etc.
💡 Here are some ideas https://www.browserling.com/tools/
To add an encoder or decoder, follow the Contributing Guide