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

PNR_Info service #13

Open
wants to merge 31 commits into
base: mass-refactoring
Choose a base branch
from
Open

PNR_Info service #13

wants to merge 31 commits into from

Conversation

alexkadyrov
Copy link
Contributor

@alexkadyrov alexkadyrov commented Jul 15, 2019

#15 related

Copy link
Collaborator

@smgladkovskiy smgladkovskiy left a comment

Choose a reason for hiding this comment

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

Помимо всего прочего, не забываем обновлять go.mod.

client/book.go Outdated Show resolved Hide resolved
client/information.go Outdated Show resolved Hide resolved
logger/logrusLogger/repository.go Outdated Show resolved Hide resolved
service/20_book.go Outdated Show resolved Hide resolved
service/21_cancel.go Outdated Show resolved Hide resolved
structs/pnr/retrieve/response.go Outdated Show resolved Hide resolved
structs/pnr/retrieve/v11.3/request/query.go Show resolved Hide resolved
structs/pnr/retrieve/v11.3/response/reply.go Outdated Show resolved Hide resolved
structs/pnr/retrieve/v19.1/request/query.go Outdated Show resolved Hide resolved
structs/pnr/retrieve/v19.1/response/reply.go Outdated Show resolved Hide resolved
@smgladkovskiy smgladkovskiy self-assigned this Jul 16, 2019
@smgladkovskiy smgladkovskiy added this to the v1.x.x milestone Jul 16, 2019
@smgladkovskiy smgladkovskiy changed the title Refactoring 2 PNR_Info service Jul 16, 2019
Copy link
Collaborator

@smgladkovskiy smgladkovskiy left a comment

Choose a reason for hiding this comment

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

Ещё немного замечаний

import (
"github.com/tmconsulting/amadeus-golang-sdk/structs/pnr/retrieve"
"github.com/tmconsulting/amadeus-golang-sdk/utils"
"gitlab.teamc.io/tm-consulting/tmc24/avia/layer3/amadeus-agent-go/configuration"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это явно лишнее

// This data element is used to qualify the company code, to identify the industry business it belongs.
TravelSector string `xml:"travelSector"`

// This data element is used to convey the company code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вынести формат времени в свойства сервиса и опицией ооверрайдить при необходимости

client/book.go Outdated
@@ -7,7 +7,7 @@ import (
"github.com/tmconsulting/amadeus-golang-sdk/structs/fare/pricePNRWithBookingClass/v14.1/response"
"github.com/tmconsulting/amadeus-golang-sdk/structs/pnr/addMultiElements/v11.3"
"github.com/tmconsulting/amadeus-golang-sdk/structs/pnr/cancel/v11.3"
"github.com/tmconsulting/amadeus-golang-sdk/structs/pnr/reply/v11.3"
PNR_Reply_v11_3_response "github.com/tmconsulting/amadeus-golang-sdk/structs/pnr/retrieve/v11.3/response"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лишний алиас

* circle ci tests

* build and test badges
badges correction
@smgladkovskiy
Copy link
Collaborator

rebase for build and tests coverage

type mapItineraries map[int]*Itinerary

// Check validate search request
func (request *SearchRequest) Check() (err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Разбивай чек на куски:

  • проверка на пустые данные
  • проверка на согласованность типа маршрута и itineraries
  • проверка пассажиров
  • проверка и работа с itineraries

Все их (функции) покрыть тестами.

return nil
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

убрать комментированные данные

*/

// SearchResponse structure of Search response
type SearchResponse struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

структуры запросов и ответов выделить и разнести по файлам, дабы читались проще

UnitNumberDetail: []*NumberOfUnitDetailsType_260583C{
{
NumberOfUnits: formats.NumericInteger_Length1To6(request.Passengers.ADT + request.Passengers.CHD),
TypeOfUnit: formats.AlphaNumericString_Length1To3("PX"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут и далее -- может в константы эти штуки выносить с добавлением того, что может быть ещё, помимо тех параметров, что тут указаны? Когда потребуется регулировать структуру запроса -- они будут вынесены в параметры request'а


paxID := 1
if request.Passengers.ADT > 0 {
var travellers []*TravellerDetailsType
Copy link
Collaborator

Choose a reason for hiding this comment

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

добавление пассажира -- в единую функцию с передачей параметра типа пассажира. Добавление пакса сделать проходом по мапе.

Copy link
Collaborator

Choose a reason for hiding this comment

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

и тесты на это

@@ -0,0 +1,35 @@
package search
Copy link
Collaborator

Choose a reason for hiding this comment

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

что пустое?

@@ -0,0 +1,122 @@
package search
Copy link
Collaborator

Choose a reason for hiding this comment

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

Что пустое?

"time"
)

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

не нужный код


func (reply *Response) ToCommon(request *search.SearchRequest) (*search.SearchResponse, error) {

return ParseReply(request, reply)
Copy link
Collaborator

Choose a reason for hiding this comment

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

а зачем делать тут функцию, если можно было бы содержимое той функции тут отразить?

var segments = make(map[string]*structsCommon.Flight)

// Recommendation details
for _, recReply := range reply.Recommendation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

вот это бы зарефакторить, разбив на более мелкие составляющие, начиная с самого глубокого цикла по дереву вложённости

@@ -164,6 +164,8 @@ func (s *SOAP4Client) Call(soapUrl, soapAction, messageId string, query, reply i
return nil, err
}

fmt.Println("XML Response: ", string(rawbody))
Copy link
Collaborator

Choose a reason for hiding this comment

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

это в логи нужно. дебагом

@@ -38,3 +41,9 @@ func SetLogger(l logger.LogWriter) Option {
c.service.Client.Logger = logger.NewLogger(l)
}
}

func SetConfig(config configuration.ConfigType) Option {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не надо так. Опции -- для опций. Тут не нужно передавать структурой целой конфиг. Если нужен формат -- сделай "setTimeFormat" функцию и всё. По-дефолту формат в свойство клиента и переопределяй опцией.

@@ -1,126 +1,14 @@
package configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

не нужен конфиг тут от слова совсем!

@@ -630,7 +630,7 @@ func ParseReply(request *search.SearchRequest, reply *Response) (*search.SearchR

var recommendation = &search.Recommendation{
ID: recommendationID,
Provider: configuration.Provider,
Provider: configuration.Config.Provider,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Конфиг не нужен. из свойств клиента брать.

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