Skip to content

Skeleton app farhan#60

Draft
FarhanOhe03 wants to merge 17 commits into
devfrom
skeleton-app-farhan
Draft

Skeleton app farhan#60
FarhanOhe03 wants to merge 17 commits into
devfrom
skeleton-app-farhan

Conversation

@FarhanOhe03
Copy link
Copy Markdown
Collaborator

Description

Changes Made

Created a reactor to get current weather from hard coded values.

How to Test

Call the reactor in pixel with city and state
Should return the temperature hard coded with the first letter of the city name as current temperature.

Notes

@tevanburen tevanburen marked this pull request as draft March 10, 2026 16:27
Copy link
Copy Markdown
Collaborator

@tevanburen tevanburen left a comment

Choose a reason for hiding this comment

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

  1. Rather than having a helper method, just have doExecute
  2. Get rid of the javadoc style comments
  3. Instead of the long switch, have it be an array of answers that you index into
  4. Remove the CurrentWeather reactor
  5. Why do you have a change in pnpm-lock?

Copy link
Copy Markdown
Collaborator

@tevanburen tevanburen left a comment

Choose a reason for hiding this comment

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

Looking good

  1. Can we keep the tab size to 2 spaces for consistency
  2. Let's update the forecasts using copilot so they flow more like english sentences ("The high today is: 75, The low today is: 55, Description: "Pleasant and sunny with a light breeze."" -> "Expect a pleasant and sunny day with a light breeze. The high will reach 75°F and the low will dip to 55°F.")
  3. " Here is the current conditions for " + city + ": " + forecast + "." is clunky - again, have it flow more naturally
  4. Let's move the forecast array to the bottom to reduce clutter

@tevanburen
Copy link
Copy Markdown
Collaborator

Nitpicky but can we keep the tab size to 2 for consistency

return "The city for which to retrieve the weather information";
}
return null;
return super.getDescriptionForKey(key);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where are these changes from?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That change came when I changed from switch to the current string setup we have now. Got the change from copilot just because returning null is less ideal ( what copilot said) but I can revert to that.

https://github.com/SEMOSS/Template/pull/60/changes/BASE..7192fd42f42338ad6eff282b4a61b99418365bb2

you can see where the change happened here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@tlokeshrao is this valid or do we want null

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Change back to CITY_KEY.equals(key) to avoid a null pointer error

return "The city for which to retrieve the weather information";
}
return null;
return super.getDescriptionForKey(key);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Change back to CITY_KEY.equals(key) to avoid a null pointer error

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.

4 participants