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

Added upper limits of required dependencies #176

Merged
merged 7 commits into from
Nov 20, 2023

Conversation

DanRCM
Copy link
Contributor

@DanRCM DanRCM commented Aug 18, 2023

#174
Issue 1: 'ghc-options: -O2' is rarely needed:
Time tests were conducted using time cabal build, yielding the following results:
Without using "-O2":
image

With the use of "-O2":
image

From these results, we can see that the use of "-O2" doesn't actually provide any time-related benefits.

Issue 2: Missing Upper Bounds in Dependency Packages
Upper bounds were added to the dependencies that required them: "-compat -containers -directory -hspec -shellwords -text". The gen-bounds tool was used, as suggested. Tests were conducted via stack to assess the compatibility of each of the packages with the upper bounds I set, based on the proposed suggestions using the gen-bounds tool."

@CristhianMotoche
Copy link
Collaborator

Thank you so much for your contribution @DanRCM LGTM!

dotenv.cabal Outdated
Comment on lines 69 to 71
, megaparsec >= 9.2.2 && <= 9.4.1
, process >= 1.6.16 && < 1.7
, text >= 1.2.5 && < 1.3
Copy link
Collaborator

@CristhianMotoche CristhianMotoche Sep 1, 2023

Choose a reason for hiding this comment

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

@DanRCM I see we're also using these dependencies in the library. It's not necessary to define the boundaries for every dependency in library, executable, and test-suite. It's good practice to define them in one section it once and the boundaries will automatically be inherited by other sections.

dotenv.cabal Outdated
Comment on lines 122 to 125
, megaparsec >= 9.2.2 && <= 9.4.1
, hspec >= 2.9.7 && <= 2.11.4
, process >= 1.6.16 && < 1.7
, text >= 1.2.5 && < 1.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment above also applies here.

stack.yaml Outdated
Comment on lines 2 to 3


Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove these extra lines.

@CristhianMotoche
Copy link
Collaborator

@DanRCM Let's specify the boundaries of the common dependencies only in the library section. The boundaries will be automatically inherited to the other sections.

dotenv.cabal Outdated
, process >= 1.6.3.0 && < 1.7
, shellwords >= 0.1.3.0
, text
build-depends: base >= 4.16.4 && < 4.17
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DanRCM Updating base version mean that you're limiting the version of GHC that is compatible with this library. You can check the relation between GHC and base here. dotenv is tested against GHC 8.10 (base >= 4.14). Let's not change that boundary.

Copy link
Collaborator

@CristhianMotoche CristhianMotoche left a comment

Choose a reason for hiding this comment

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

@DanRCM It looks good. Let's not update the boundaries of base, though.

@DanRCM
Copy link
Contributor Author

DanRCM commented Sep 19, 2023

@CristhianMotoche Hi, thanks for your recommendation, the changes have been made.

@CristhianMotoche
Copy link
Collaborator

Hello @DanRCM Thanks for addressing the suggestions. It seems something in the recent changes has caused issues when building the package. I think you need to lower the boundaries for text.

@CristhianMotoche
Copy link
Collaborator

Hello @DanRCM Do you need any help on this PR?

@DanRCM
Copy link
Contributor Author

DanRCM commented Oct 13, 2023

Hello @DanRCM Do you need any help on this PR?

Hello, excuse me, I have been really busy, but I'll continue working in your project, thank you for your help, I'll do the changues inmediately

@DanRCM
Copy link
Contributor Author

DanRCM commented Oct 14, 2023

Hello @CristhianMotoche the changes have been made, I have been taking your advices to lower the text boundaries.

@DanRCM
Copy link
Contributor Author

DanRCM commented Oct 25, 2023

@CristhianMotoche
Hello, could you tell me what you suspect about what could be giving me problems with the PR? I need help

@CristhianMotoche
Copy link
Collaborator

Hello @DanRCM It seems there are some issues with the changed boundaries. Let me double check your PR locally and I'll keep you posted.

@DanRCM
Copy link
Contributor Author

DanRCM commented Oct 29, 2023

Hello @DanRCM It seems there are some issues with the changed boundaries. Let me double check your PR locally and I'll keep you posted.

ok, thanks for keeping me in mind, it's a pleasure

@CristhianMotoche CristhianMotoche merged commit 92f8116 into stackbuilders:master Nov 20, 2023
6 checks passed
@CristhianMotoche
Copy link
Collaborator

@all-contributors please add @DanRCM for code

Copy link
Contributor

@CristhianMotoche

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

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.

2 participants