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

fix: Respect contracts directory defined in adonisjrc.json #81

Merged
merged 4 commits into from
Aug 11, 2023
Merged

fix: Respect contracts directory defined in adonisjrc.json #81

merged 4 commits into from
Aug 11, 2023

Conversation

TiBianMod
Copy link
Contributor

@TiBianMod TiBianMod commented Nov 19, 2022

The node ace configure @adonisjs/mail ignores the defined contracts directory from the user inside .adonisjrc.json

This PR will respect the contracts directory defined by the user inside .adonisjrc.json and will fallback to default if not present.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Further comments

@thetutlage point me to right direction for adding tests if necessary.
I can't find related tests for node ace configure @adonisjs/mail

@stale
Copy link

stale bot commented Feb 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Abandoned Dropped and not into consideration label Feb 2, 2023
@TiBianMod
Copy link
Contributor Author

ping @thetutlage

@stale stale bot removed the Status: Abandoned Dropped and not into consideration label Feb 5, 2023
@@ -126,7 +126,8 @@ export default async function instructions(
/**
* Create the mail contracts file
*/
const contractsPath = app.makePath('contracts/mail.ts')
const contractsDir = app.directoriesMap.get('contracts') || 'contracts'
Copy link
Member

Choose a reason for hiding this comment

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

Can we use path.join instead?

Suggested change
const contractsDir = app.directoriesMap.get('contracts') || 'contracts'
const contractsDirectory = app.directoriesMap.get('contracts') || 'contracts'
const contractPath = join(contractsDirectory, 'mail.ts')

@@ -141,7 +142,7 @@ export default async function instructions(
sparkpost: mailDrivers.includes('sparkpost'),
})
.commit()
sink.logger.action('create').succeeded('contracts/mail.ts')
sink.logger.action('create').succeeded(`${contractsDir}/mail.ts`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sink.logger.action('create').succeeded(`${contractsDir}/mail.ts`)
sink.logger.action('create').succeeded(contractPath)

TiBianMod and others added 2 commits February 6, 2023 20:25
Co-authored-by: Romain Lanz <2793951+RomainLanz@users.noreply.github.com>
Co-authored-by: Romain Lanz <2793951+RomainLanz@users.noreply.github.com>
@TiBianMod TiBianMod requested a review from RomainLanz February 6, 2023 19:27
@TiBianMod
Copy link
Contributor Author

I have reverted the changes back to initial PR.
Let's keep consistency with the line 123-124

** Also in your commit was an error on line 131 :-(

@stale
Copy link

stale bot commented Apr 14, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Abandoned Dropped and not into consideration label Apr 14, 2023
@stale stale bot closed this Apr 26, 2023
@RomainLanz RomainLanz reopened this Apr 30, 2023
@stale stale bot removed the Status: Abandoned Dropped and not into consideration label Apr 30, 2023
@RomainLanz RomainLanz requested a review from thetutlage April 30, 2023 08:50
@stale
Copy link

stale bot commented Aug 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Abandoned Dropped and not into consideration label Aug 10, 2023
@TiBianMod TiBianMod closed this Aug 10, 2023
@thetutlage thetutlage reopened this Aug 11, 2023
@stale stale bot removed the Status: Abandoned Dropped and not into consideration label Aug 11, 2023
@thetutlage thetutlage merged commit e953f0e into adonisjs:develop Aug 11, 2023
@thetutlage
Copy link
Member

@TiBianMod Thanks for the PR and sorry for taking that long :)

@TiBianMod
Copy link
Contributor Author

@thetutlage no problem mate 😄

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.

3 participants