console-err-msg #430

Rispectech · 2021-10-02T16:44:41Z

In order to prevent redundancy in each component, I created a new js file (errorfunc.js) which contains all the error functions I used apart from occasional exceptions where it couldn't be used

in errorfunc.js , checkMissingValueObj checks whether the values are present or not

vercel · 2021-10-02T16:44:45Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/saadpasta/developer-folio/Sis1cT2Xy6miZfydqQGGSvVqrAyv
✅ Preview: https://developer-folio-git-fork-rispectech-console-error-msg-saadpasta.vercel.app

kartikcho

I took a quick look over the changes and it seems you understood the issues incorrectly. The issue #212 says to add error messages for only third party components, i.e GitHub repos, Twitter embed, podcasts and the GitHub contact components.

Throwing errors for missing fields would be counter intuitive as we also want users to have the option to keep fields blank if they wish to do so. Lots of errors in the console blowing up in people's face will make them feel they're doing something wrong when it's just an empty field.

Also looking at the implementation, you're using logs instead of errors which isn't the best way to warn someone.

kartikcho · 2021-10-03T11:14:15Z

src/components/educationCard/EducationCard.js

@@ -16,6 +20,9 @@ export default function EducationCard({school}) {
      : null;
  };
  const {isDark} = useContext(StyleContext);
+
+  if (!school.logo)
+    console.log(`Image of schoolName ${school.name} is not available`);


We can keep this in as it seems helpful but change it to console.error('Image of ${school.name} is missing in education section');

kartikcho · 2021-10-03T11:15:52Z

src/components/errorfunc.js

@@ -0,0 +1,48 @@
+//function to check whether the url is correct


You'll probably need to scrap all of this code and it's usage in other components as it isn't what is required currently :(

Rispectech · 2021-10-03T14:27:42Z

I took a quick look over the changes and it seems you understood the issues incorrectly. The issue #212 says to add error messages for only third party components, i.e GitHub repos, Twitter embed, podcasts and the GitHub contact components.

Throwing errors for missing fields would be counter intuitive as we also want users to have the option to keep fields blank if they wish to do so. Lots of errors in the console blowing up in people's face will make them feel they're doing something wrong when it's just an empty field.

Also looking at the implementation, you're using logs instead of errors which isn't the best way to warn someone.

okay, so you want the error using throw error instead of log. right will do this one
second right using the error function for all the components intuitive so fix this as well

Rispectech · 2021-10-03T15:56:25Z

I have converted the console messages in third party components to error messages and removed them from the rest
errorfunc.js deleted as well
@kartikcho please check and review the changes
thanks

kartikcho

There are a lot of changes to make so please see all review comments and double check to not leave any changes not relevant to the PR.
Also, run prettier and fix all the code formatting inconsistencies.

kartikcho · 2021-10-07T16:42:32Z

src/components/blogCard/BlogCard.js

      return;
    }
+


remove this extra line

kartikcho · 2021-10-07T16:44:25Z

src/components/blogCard/BlogCard.js

    var win = window.open(url, "_blank");
    win.focus();
  }

  return (
-    <div onClick={() => openUrlInNewTab(blog.url)}>
+    <div onClick={() => openUrlInNewTab(blog.url, blog.img, blog.title)}>


This would work incorrectly to log image path so we don't need to pass blog.img here.

kartikcho · 2021-10-07T16:49:26Z

src/components/experienceCard/ExperienceCard.js

-        ))
-      : null;
+    if (!descBullets) return null;
+    else


We don't need an if-else statement here since it will be evaluated to null by the ternary operator if it's false. Adding this would be an unnecessary extra step so let's revert this.

kartikcho · 2021-10-07T16:49:53Z

src/components/educationCard/EducationCard.js

@@ -7,6 +7,7 @@ export default function EducationCard({school}) {
  const imgRef = createRef();

  const GetDescBullets = ({descBullets}) => {
+    if (!descBullets) return null;


Same as ExperienceCard.js, let's revert this change.

You missed reverting these changes.

kartikcho · 2021-10-07T16:52:08Z

src/components/achievementCard/AchievementCard.js

    if (!url) {
+      console.log(`URl for ${name} not found `);


nit: extra space at the end of the string and use 'URL'

kartikcho · 2021-10-07T17:34:35Z

src/containers/podcast/Podcast.js

@@ -6,6 +6,10 @@ import StyleContext from "../../contexts/StyleContext";

 export default function Podcast() {
  const {isDark} = useContext(StyleContext);
+
+  if (!podcastSection)
+    throw new Error("Podcast objects for Podcast section is missing");


Let's stick to throwing errors only in console throughout the app so use console.error

I will revert this but I wanted to discuss whether if throwing an error is a better choice than using the console. error since we are using map function and it is better to let the user know that the object is missing since we will get an error anyway if it is undefined and we using the map

We return just below this line (L13) if the object is undefined or display is set to false so it will be covered.

kartikcho · 2021-10-07T17:37:49Z

src/containers/profile/Profile.js

@@ -21,7 +22,7 @@ export default function Profile() {
            if (result.ok) {
              return result.json();
            }
-            console.error(result);
+            throw new Error("Couldn't fetch profile.json data");


We don't need any error here since we're catching it below

kartikcho · 2021-10-07T17:39:18Z

src/containers/profile/Profile.js

            openSource.showGithubProfile = "false";
+            throw new Error(
+              `${error} occured when tried to fetch profile data`


revert this and go back to using console.error

kartikcho · 2021-10-07T17:40:47Z

src/containers/projects/Projects.js

@@ -50,6 +50,11 @@ export default function Projects() {
          <h1 className="project-title">Open Source Projects</h1>
          <div className="repo-cards-div-main">
            {repo.map((v, i) => {
+              if (!v) {
+                throw new Error(
+                  `Github Object for repository number : ${i + 1} is undefined`


See object v in GithubRepoCard and use repository name instead of number in the error.
Also, use console.error

actually, I tried doing this but it complains you can not access the .node.id property when u pass props to the GithubRepocard so we have to display the error before we pass the props
Secondly, the name is present in the object v in the node key so I don't think there is any other way to get the name of the repo

I meant display repo name using v.node.name here instead of repo id if possible, since that would make the message more verbose.

kartikcho · 2021-10-07T17:43:39Z

src/containers/twitter-embed/twitter.js

@@ -24,6 +24,9 @@ export default function Twitter() {
  if (!twitterDetails.display) {
    return null;
  }
+  if (!twitterDetails.userName) {
+    throw new Error("Twitter details for twitter section are missing ");


use console.error

change error to 'Twitter username for Twitter section is missing'

Remove extra space at the end of string

Rispectech · 2021-10-08T05:32:19Z

sorry @kartikcho for being such a pain making you request changes again and again
I got confused with throw and console when you last commented so made these mistakes so sorry for that
I have made the changes so please review and check
thanks

kartikcho

We're almost there and can get this in soon, just need to address a few more comments. Make sure to not leave any extra lines committed and run formatting just to be extra sure.

Also, don't feel bad if your PR is taking more rounds of reviews, we're all learning here!

kartikcho · 2021-10-08T17:24:05Z

src/components/educationCard/EducationCard.js

@@ -7,6 +7,7 @@ export default function EducationCard({school}) {
  const imgRef = createRef();

  const GetDescBullets = ({descBullets}) => {
+    if (!descBullets) return null;


You missed reverting these changes.

kartikcho · 2021-10-08T17:25:21Z

src/components/experienceCard/ExperienceCard.js

@@ -18,6 +18,7 @@ export default function ExperienceCard({cardInfo, isDark}) {
  }

  const GetDescBullets = ({descBullets, isDark}) => {
+    if (!descBullets) return null;


Missed this too, remove this as we're checking for descBullets using the ternary operator below.

kartikcho · 2021-10-08T17:25:43Z

src/components/githubProfileCard/GithubProfileCard.js

@@ -6,11 +6,13 @@ import emoji from "react-easy-emoji";
 import {Fade} from "react-reveal";

 export default function GithubProfileCard({prof}) {
+  if (!prof) throw new Error("GithubProfileObject missing ");


Remove all changes from this file then @Rispectech

kartikcho · 2021-10-08T17:26:25Z

src/components/githubRepoCard/GithubRepoCard.js

    if (!url) {
+      console.log(`Url in ${name} is undefined`);


nit: Overlooked this 'URL' :P

kartikcho · 2021-10-08T17:27:02Z

src/containers/profile/Profile.js

@@ -7,6 +7,7 @@ const renderLoader = () => <Loading />;
 const GithubProfileCard = lazy(() =>
  import("../../components/githubProfileCard/GithubProfileCard")
 );
+


Extra line committed

kartikcho · 2021-10-08T17:29:00Z

src/containers/profile/Profile.js

            openSource.showGithubProfile = "false";
+            console.error(`${error} occured when trying to fetch profile`);


The error that would be caught here would be a complete error message in itself so we don't need to wrap it in a message here as it would make the error log confusing.

kartikcho · 2021-10-08T17:32:33Z

src/containers/projects/Projects.js

@@ -50,6 +50,11 @@ export default function Projects() {
          <h1 className="project-title">Open Source Projects</h1>
          <div className="repo-cards-div-main">
            {repo.map((v, i) => {
+              if (!v) {
+                throw new Error(
+                  `Github Object for repository number : ${i + 1} is undefined`


I meant display repo name using v.node.name here instead of repo id if possible, since that would make the message more verbose.

kartikcho · 2021-10-08T17:33:26Z

src/containers/twitter-embed/twitter.js

@@ -24,6 +24,9 @@ export default function Twitter() {
  if (!twitterDetails.display) {
    return null;
  }
+  if (!twitterDetails.userName) {
+    console.error("Twitter username for twitter section are missing");


nit: 'Twitter username for Twitter section is missing'

Rispectech · 2021-10-09T05:23:38Z

We're almost there and can get this in soon, just need to address a few more comments. Make sure to not leave any extra lines committed and run formatting just to be extra sure.

Also, don't feel bad if your PR is taking more rounds of reviews, we're all learning here!

i don't mind getting reviews on my pr it just shows where I am lacking. it's just that constantly disturbing you is bugging me 😅

Rispectech · 2021-10-09T05:34:05Z

I have made most of the changes along with the repo name but I am sure it won't work as if v is undefined then v.node won't be accessible as we will get the error "cant access property node of undefined" it was same in case of v.node.id that's why I
used the repository number.
here is a working sample

but I case of repository number it doesn't give this error and console the error with the rest of the messages

kartikcho

Just a couple of small changes to address and also revert the GitHub repo error message to how it was before, using repo index.
Almost there!

kartikcho · 2021-10-12T15:18:48Z

src/components/githubProfileCard/GithubProfileCard.js

@@ -11,6 +11,7 @@ export default function GithubProfileCard({prof}) {
  } else {
    prof.hireable = "No";
  }
+


remove this extra line

kartikcho · 2021-10-12T15:21:13Z

src/containers/podcast/Podcast.js

@@ -6,6 +6,10 @@ import StyleContext from "../../contexts/StyleContext";

 export default function Podcast() {
  const {isDark} = useContext(StyleContext);
+
+  if (!podcastSection)
+    console.error("Podcast objects for Podcast section is missing");


Let's change this to 'podcastSection' object for Podcast section is missing to make it a bit nicer

so if I am understanding right, you want the variable podcastSection which was in the if statement in the string right ? .
assuming so it won't work since this if block fires only when podcastSection is null so if we pull it in the string
the output we get would be
"null / undefined objects for Podcast section is missing "

here is the working sample
I have made the changes though

kartikcho · 2021-10-12T15:21:56Z

src/containers/podcast/Podcast.js

@@ -26,6 +30,11 @@ export default function Podcast() {
        </div>
        <div className="podcast-main-div">
          {podcastSection.podcast.map((podcastLink, i) => {
+            if (!podcastLink) {
+              console.log(
+                `podcast link for ${podcastSection.title} is missing`


nit: Use a capital P at the start of the sentence :P

kartikcho · 2021-10-12T15:23:59Z

src/containers/projects/Projects.js

@@ -50,6 +50,9 @@ export default function Projects() {
          <h1 className="project-title">Open Source Projects</h1>
          <div className="repo-cards-div-main">
            {repo.map((v, i) => {
+              if (!v) {
+                console.error(`Github Object for ${v.node.name} is undefined`);


Hmm this should work but I'm just speculating without digging deep into it.
Revert it the previous error message you proposed using repo index for now.

yeah, I know that my solution was messy and not informative but I couldn't figure out how to access the name since v is undefined. i would like to better than this but cant figure out how to do so

Rispectech · 2021-10-13T05:11:11Z

Just a couple of small changes to address and also revert the GitHub repo error message to how it was before, using repo index. Almost there!

I have done these changes and I wasn't so sure about the GitHub repo error so if are talking about the error messages in getRepoData in project.js in container, I have done that
thanks

kartikcho · 2021-10-13T05:31:15Z

src/containers/podcast/Podcast.js

@@ -6,6 +6,10 @@ import StyleContext from "../../contexts/StyleContext";

 export default function Podcast() {
  const {isDark} = useContext(StyleContext);
+
+  if (!podcastSection)
+    console.error(`${podcastSection} object for Podcast section is missing`);


Sorry for being confusing before, I didn't mean to print the object in the console, just the variable name.
So it should look like console.error('podcastSection object for Podcast section is missing');

No worries. i have done the changes

kartikcho

Everything looks good to go, thanks!

kartikcho · 2021-10-14T11:53:30Z

@allcontributors add @Rispectech for code

allcontributors · 2021-10-14T11:53:39Z

@kartikcho

I've put up a pull request to add @Rispectech! 🎉

* console-err-msg * fixed console to error message * requested changes * . * githubprofilecard-revert * required changes * changes-required * changes-required

console-err-msg

ec42d58

vercel bot deployed to Preview October 2, 2021 16:44 View deployment

Rispectech mentioned this pull request Oct 2, 2021

Add custom error messages for console #212

Closed

kartikcho requested changes Oct 3, 2021

View reviewed changes

fixed console to error message

fb1a6b4

vercel bot deployed to Preview October 3, 2021 15:54 View deployment

kartikcho requested changes Oct 7, 2021

View reviewed changes

requested changes

a971013

vercel bot deployed to Preview October 8, 2021 05:30 View deployment

.

d5da4e6

vercel bot deployed to Preview October 8, 2021 05:37 View deployment

githubprofilecard-revert

bf78c2b

vercel bot deployed to Preview October 8, 2021 17:31 View deployment

kartikcho requested changes Oct 8, 2021

View reviewed changes

required changes

3f2df37

vercel bot deployed to Preview October 9, 2021 05:33 View deployment

kartikcho requested changes Oct 12, 2021

View reviewed changes

changes-required

663aeff

vercel bot deployed to Preview October 13, 2021 05:12 View deployment

kartikcho requested changes Oct 13, 2021

View reviewed changes

changes-required

a84cbc9

vercel bot deployed to Preview October 13, 2021 05:35 View deployment

kartikcho approved these changes Oct 14, 2021

View reviewed changes

kartikcho merged commit cdb9998 into saadpasta:master Oct 14, 2021

allcontributors bot mentioned this pull request Oct 14, 2021

docs: add Rispectech as a contributor for code #436

Merged

		@@ -0,0 +1,48 @@
		//function to check whether the url is correct

		openSource.showGithubProfile = "false";
		console.error(`${error} occured when trying to fetch profile`);

console-err-msg #430

console-err-msg #430

Conversation

Rispectech commented Oct 2, 2021

vercel bot commented Oct 2, 2021 • edited Loading

kartikcho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rispectech commented Oct 3, 2021 • edited Loading

Rispectech commented Oct 3, 2021 • edited Loading

kartikcho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rispectech Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rispectech commented Oct 8, 2021

kartikcho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rispectech commented Oct 9, 2021 • edited Loading

Rispectech commented Oct 9, 2021

kartikcho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rispectech commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikcho left a comment

Choose a reason for hiding this comment

kartikcho commented Oct 14, 2021

allcontributors bot commented Oct 14, 2021

vercel bot commented Oct 2, 2021 •

edited

Loading

Rispectech commented Oct 3, 2021 •

edited

Loading

Rispectech commented Oct 3, 2021 •

edited

Loading

Rispectech Oct 8, 2021 •

edited

Loading

Rispectech commented Oct 9, 2021 •

edited

Loading