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

Use fetch instead of XMLHttpRequest in weatherprovider #2935

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

rejas
Copy link
Collaborator

@rejas rejas commented Oct 6, 2022

small update to the fetchData method to use the fetch helper instead of the old XCMLHttpRequest.
Also fixes some typos :-)

veeck and others added 2 commits October 6, 2022 13:08
also remove unused method parameter, is GET all the time
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2022

Codecov Report

Merging #2935 (d1a5f63) into develop (a86e27a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2935   +/-   ##
========================================
  Coverage    63.82%   63.82%           
========================================
  Files            9        9           
  Lines          293      293           
========================================
  Hits           187      187           
  Misses         106      106           
Impacted Files Coverage Δ
js/fetch.js 16.66% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@khassel khassel merged commit d5e855d into MagicMirrorOrg:develop Oct 6, 2022
@rejas
Copy link
Collaborator Author

rejas commented Oct 6, 2022

Thanks for merging so quick @khassel. But could we agree that only the maintainer who created a PR merges it (of course only after one of the other maintainers approves it)? Or should we keep it strict to the "if its not really finished, do a draft PR" policy? What do you think?

@rejas rejas deleted the fetch_weather branch October 6, 2022 20:05
@sdetweil
Copy link
Collaborator

sdetweil commented Oct 6, 2022

what is a 'draft' pr? never seen one til the other day.

I don't think I should be the one merging MY submitted PR's..

@khassel
Copy link
Collaborator

khassel commented Oct 6, 2022

Or should we keep it strict to the "if its not really finished, do a draft PR" policy?

yes, if not ready --> draft, if discussions are coming up --> draft

But could we agree that only the maintainer who created a PR merges it (of course only after one of the other maintainers approves it)?

I was thinking of the opposite, never merge your own PR ...

@khassel
Copy link
Collaborator

khassel commented Oct 6, 2022

what is a 'draft' pr? never seen one til the other day.

this is a PR which is not ready for merge (we have 2 at the moment here). You can open it as draft for discussing stuff or change later to draft if someone has suggestions or code must be refactored before merging.

You can't merge draft PR's.

@rejas
Copy link
Collaborator Author

rejas commented Oct 6, 2022

what is a 'draft' pr? never seen one til the other day.

but now you have I assume? when creating a PR you can mark it as a draft (so basically WorkInProgress)

I don't think I should be the one merging MY submitted PR's..

so new rule:

  • never merge your own PRs
  • never merge without someone else having approved?

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 6, 2022

I'm good with that .. (rules)

I know what 'draft' means, just never seen how to select it..
should always be draft first , review approval changes to not draft

@khassel
Copy link
Collaborator

khassel commented Oct 6, 2022

o.k. for me too

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 6, 2022

i just tried to create a PR, but don't see a mechanism for draft..

@khassel
Copy link
Collaborator

khassel commented Oct 6, 2022

Existing PR see picture below:

grafik

AFAIK this property is also visible when creating a new one.

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 6, 2022

I was trying to create a new one.. we shall see

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 6, 2022

there it is . never noticed the dropdown!..

Screenshot at 2022-10-06 15-40-17

@rejas
Copy link
Collaborator Author

rejas commented Oct 7, 2022

there it is . never noticed the dropdown!..

yes, its not the best UI decision from github....

@MichMich MichMich mentioned this pull request Jan 1, 2023
MichMich added a commit that referenced this pull request Jan 1, 2023
## [2.22.0] - 2023-01-01

Thanks to: @angeldeejay, @buxxi, @dariom, @dWoolridge,
@KristjanESPERANTO, @MagMar94, @naveensrinivasan, @retroflex, @SkySails
and @tom.

Special thanks to @khassel, @rejas and @sdetweil for taking over most
(if not all) of the work on this release as project collaborators. This
version would not be there without their effort. Thank you!

### Added

- Added test for remoteFile option in compliments module
- Added hourlyWeather functionality to Weather.gov weather provider
- Removed weatherEndpoint definition from weathergov.js (not used)
- Added css class names "today" and "tomorrow" for default calendar
- Added Collaboration.md
- Added new github action for dependency review (#2862)
- Added a WeatherProvider for Open-Meteo
- Added Yr as a weather provider
- Added config options "ignoreXOriginHeader" and
"ignoreContentSecurityPolicy"

### Removed

- Removed usage of internal fetch function of node until it is more
stable

### Updated

- Cleaned up test directory (#2937) and jest config (#2959)
- Wait for all modules to start before declaring the system ready
(#2487)
- Updated e2e tests (moved `done()` in helper functions) and use es6
syntax in all tests
- Updated da translation
- Rework weather module
- Make sure smhi provider api only gets a maximum of 6 digits
coordinates (#2955)
  - Use fetch instead of XMLHttpRequest in weatherprovider (#2935)
  - Reworked how weatherproviders handle units (#2849)
  - Use unix() method for parsing times, fix suntimes on the way (#2950)
  - Refactor conversion functions into utils class (#2958)
- The `cors`-method in `server.js` now supports sending and recieving
HTTP headers
- Replace `&hellip;` by `…`
- Cleanup compliments module
- Updated dependencies including electron to v22 (#2903)

### Fixed

- Correctly show apparent temperature in SMHI weather provider
- Ensure updatenotification module isn't shown when local is _ahead_ of
remote
- Handle node_helper errors during startup (#2944)
- Possibility to change FontAwesome class in calendar, so icons like
`fab fa-facebook-square` works.
- Fix cors problems with newsfeed articles (as far as possible), allow
disabling cors per feed with option `useCorsProxy: false` (#2840)
- Tests not waiting for the application to start and stop before
starting the next test
- Fix electron tests failing sometimes in github workflow
- Fixed gap in clock module when displayed on the left side with
displayType=digital
- Fixed playwright issue by upgrading to v1.29.1 (#2969)

Signed-off-by: naveen <[email protected]>
Co-authored-by: Karsten Hassel <[email protected]>
Co-authored-by: Malte Hallström <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: dWoolridge <[email protected]>
Co-authored-by: Johan <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Magnus <[email protected]>
Co-authored-by: Naveen <[email protected]>
Co-authored-by: buxxi <[email protected]>
Co-authored-by: Thomas Hirschberger <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: Andrés Vanegas Jiménez <[email protected]>
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