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

Issue 572, 589: Fix dependency resolution #592

Merged
merged 9 commits into from
Mar 10, 2021
Merged

Issue 572, 589: Fix dependency resolution #592

merged 9 commits into from
Mar 10, 2021

Conversation

senier
Copy link
Member

@senier senier commented Mar 4, 2021

Description

Close #572
Close #589

Review Checklist

Reviewer: @treiher

Implementation

  • Does this code change accomplish what it is supposed to do?
  • Can this solution be simplified?
  • Does this change add unwanted compile-time or run-time dependencies?
  • Was a framework, API, library, service used that should not be used?
  • Was a framework, API, library, service not used that could improve the solution?
  • Is the code at the right abstraction level?
  • Is the code modular enough?
  • Would you have solved the problem in a different way that is substantially better in terms of the code’s maintainability, readability, performance, security?
  • Does similar functionality already exist in the codebase? If so, why isn’t this functionality reused?
  • Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?

Logic Errors and Bugs

  • Can you think of any use case in which the code does not behave as intended?
  • Can you think of any inputs or external events that could break the code?

Error Handling and Logging

  • Is error handling done the correct way?
  • Should any logging or debugging information be added or removed?
  • Are error messages user-friendly?
  • Are there enough log events and are they written in a way that allows for easy debugging?
  • Is the style of error / logging messages consistent?

Dependencies

  • If this change requires updates outside of the code, like updating the documentation, configuration, readme files, was this done?
  • Might this change have any ramifications for other parts of the system, or backward compatibility?

Security and Data Privacy

  • Does this code open the software up for security vulnerabilities?
  • Does this code change reveal some secret information like keys, passwords, or usernames (e.g., checked-in credentials)?

Performance

  • Do you think this code change will impact system performance in a negative way?
  • Do you see any potential to improve the performance of the code?

Usability

  • Is the proposed solution well designed from a usability perspective?
  • Is the API well documented?
  • Is the API/UI intuitive to use?

Testing

  • Does it have enough automated tests (unit/integration/system tests)?
  • Are there some test cases, input or edge cases that should be tested in addition?

Readability

  • Was the code easy to understand?
  • Can the readability of the code be improved by smaller methods?
  • Can the readability of the code be improved by different function/method or variable names?
  • Is the code located in the right file/folder/package?
  • Do you think certain methods should be restructured to have a more intuitive control flow?
  • Is the data flow understandable?
  • Are there redundant comments?
  • Could some comments convey the message better?
  • Would additional comments make the code more understandable?
  • Could some comments be removed by making the code itself more readable?
  • Is there any commented out code?

Experts Opinion

  • Do you think a specific expert, like a security expert or a usability expert, should look over the code before it can be committed?
  • Will this code change impact different teams?
  • Should they have a say on the change as well?

rflx/specification/parser.py Outdated Show resolved Hide resolved
rflx/specification/parser.py Outdated Show resolved Hide resolved
tests/unit/specification/parser_test.py Outdated Show resolved Hide resolved
tests/unit/specification/parser_test.py Outdated Show resolved Hide resolved
tests/unit/specification/parser_test.py Outdated Show resolved Hide resolved
tests/unit/specification/parser_test.py Outdated Show resolved Hide resolved
tests/data/specs/in_p1.rflx Outdated Show resolved Hide resolved
Base automatically changed from develop to main March 9, 2021 16:44
@senier senier requested a review from treiher March 9, 2021 23:06
treiher
treiher previously approved these changes Mar 10, 2021
@senier
Copy link
Member Author

senier commented Mar 10, 2021

Rebased to main.

@senier senier requested a review from treiher March 10, 2021 14:21
@senier senier merged commit 85b7f6f into main Mar 10, 2021
@senier senier deleted the issue_572 branch March 10, 2021 15:59
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.

Detect multiple inclusion of same package Invalid dependency resolution
2 participants