Improved Error Handling for Queries, Simplified Logic, and Formatted README #21
Conversation
…ADME page i have like little to no experience with typescript. i wanted to add more features, but `ts` is different than `js`. might add a query for retrieving an array of reviews in future. let me know if there's anything to add or fix.
| ); | ||
| } catch (error: unknown) { | ||
| if (error instanceof Error) { | ||
| console.error('Error in searchSchool:', error.message); |
There was a problem hiding this comment.
here (and other places) we should handle the error or re-throw instead of ignoring it (this lets the caller decide what to do with it instead of the library deciding for them)
There was a problem hiding this comment.
I was thinking about returning a Promise.reject or we can always add back throw error.
There was a problem hiding this comment.
since this is an async function we don't have to use Promise.reject, we can just throw like normal
| }, | ||
| legacyId: 1234567 | ||
| } | ||
| */ |
There was a problem hiding this comment.
I think these commented out responses are still useful to show the basic shape of the available data (open to editing it to make it a more generalized example though)
README.md
Outdated
| ```bash | ||
| npm install @mtucourses/rate-my-professors | ||
| or | ||
| yarn add @mtucourses/rate-my-professors // recommended |
There was a problem hiding this comment.
I personally wouldn't explictly recommend Yarn as v1 has reached its end of life
There was a problem hiding this comment.
Alright, we can delete that part. No worries.
…s, changed the error handling and added `npm` package.lock
|
Hi, |
|
Okay, I pushed the new code. I added changes to the URL by adding the URL as a proxy in |
| "ts-node/register" | ||
| ] | ||
| } | ||
| "proxy": "https://www.ratemyprofessors.com" |
There was a problem hiding this comment.
sorry, not clear on why this is needed
There was a problem hiding this comment.
Okay, so the whole reason I stumbled upon your project is because I am trying to integrate RMP into my project called https://courseer.co/. When I tried to use the package, I get the CORS problem and after looking up online, one way to fix the issue is by adding a proxy apparently. I am not sure how exactly it fixes it, but it can help with the CORS problem when implementing your package in frontend code to retrieve ratings of the professor. If there is a better way, feel free to explain for both the package or my React project.
There was a problem hiding this comment.
I believe proxy is generally only used for create react apps, I don't think it has a meaning outside of that
There was a problem hiding this comment.
Interesting, but if I install your package, then it can remove the CORS issue for React apps, right? If it can remove it, then I would prefer keeping it for anyone to use in the future.
There was a problem hiding this comment.
can you link to documentation to explain how this would help?
| ```bash | ||
| # First: | ||
| # install dependencies | ||
| yarn install | ||
| # Install dependencies | ||
| npm install | ||
| ``` | ||
|
|
||
| # then: | ||
| # build in watch mode | ||
| yarn build:watch | ||
| ## Build | ||
|
|
||
| # and you can: | ||
| ```bash | ||
| # Build in watch mode | ||
| npm run build:watch | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| # run tests | ||
| yarn test | ||
| ```bash | ||
| # Run tests | ||
| npm test | ||
|
|
||
| # run tests in watch mode | ||
| yarn test:watch |
There was a problem hiding this comment.
very minor but I like the old format here haha
There was a problem hiding this comment.
Didn't you mention that yarn is not recommended, so I just used npm for all of them.
There was a problem hiding this comment.
yeah I'm fine switching to npm, just liked the old wording a bit more
| ); | ||
| } catch (error: unknown) { | ||
| if (error instanceof Error) { | ||
| console.error('Error in searchSchool:', error.message); |
There was a problem hiding this comment.
since this is an async function we don't have to use Promise.reject, we can just throw like normal
|
Looks like the changes seem accurate. We can leave the |
|
Do you know what the conflicts are? I don't have write access to fix it, so can you look into it? |
In this pull request, I have implemented several enhancements to the existing codebase, focusing on error handling for queries, simplifying logic, and reformatting the README page for better readability and understanding. Although I have limited experience with TypeScript, I have tried my best to contribute positively to the project. I also plan to potentially add a feature for retrieving an array of reviews in the future once I learn more TypeScript. Any feedback or suggestions for further improvements are highly appreciated.
List of Changes:
Error Handling: Added comprehensive error handling for queries to ensure the application can handle unexpected issues gracefully. This includes checking for malformed queries, unreachable data sources, and other possible errors that could arise during execution.
Simplified Logic: Reviewed and refactored the existing logic in the codebase to make it more efficient, readable, and maintainable. This will help other contributors to easily understand and build upon the code in the future.
README Reformatting: Restructured the README page to make it more user-friendly and informative. This includes adding clear headings and sections, improving the formatting of code snippets, and providing more context about the project and its features.
Future Plans: I plan to add a new query feature for retrieving an array of reviews in the future, as it can help users access more detailed information about a particular product or service. I am open to any suggestions or advice on how to approach this feature.
Contributor Experience: As someone with limited TypeScript experience, I would like to acknowledge that there may be room for further improvements in my contributions. I am eager to learn and grow, so please feel free to provide any feedback or suggestions to help me enhance my TypeScript skills and contribute better to this project.